Closed septs closed 4 years ago
@brettz9 I am confused about the /test
directory, don't know how to add test cases.
Just needs to go inside test/test.js
. The polyfill for Node can be set in test/test-node.js
(when running the tests in the browser, the Node-specific code in that file will be avoided). If you're still unclear, I can look to add it, though it'd help if you could come up with some code toward a test case, even with just logging instead of assertions.
@brettz9 the polyfill library not impl, Symbol.toStringTag
i create an issue: https://github.com/PeculiarVentures/node-webcrypto-ossl/issues/165
In the test/test-node.js
file, you can add the symbol onto its prototype, or wrap the class--just so that we can show some way to simulate similar behavior in Node. (Since this isn't even a Node API, in addition to helping check the code, the test environment can also serve as a demonstration of how Node use might be done, esp. if the existing Node polyfills are not complete in some way.)
Ok:
crypto
instead of CryptoKey
in the structured cloning preset since it simplifies Node polyfilling, especially given that CryptoKey
is not exported from the polyfill.If you could just make sure to run npm run eslint
for any future PRs (or use an eslint-aware IDE to be alerted during development), it'd be appreciated.
Btw, your ideas and changes have been very helpful, thank you!
@brettz9
write to .vscode/settings.json
for vscode user, on save auto format and fix all code
{
"editor.formatOnSave": false,
"editor.codeActionsOnSave": {
"source.fixAll.eslint": true
}
}
see https://code.visualstudio.com/updates/v1_23#_run-code-actions-on-save see https://code.visualstudio.com/docs/getstarted/settings
Not using VS myself, but open to a PR.
Note however that not all ESLint rules have fixers, so the linting still ought to be checked.
@brettz9 I think the test should be divided into two parts
@septs : Re: headless testing, while I'd normally be open to that, I'm not too big of a fan of it these days as I am in China working at home with a quite slow and unreliable connection, and any reinstall of the large Chromium, etc. ends up taking forever for me, the connection drops, etc. While I am maintaining this, I want to be able to work with the tests.
What is your motivation for separating tests? This approach avoids duplication as there is very little that differs between the two when the Node environment is set up as we have, and our separation follows standard mocha practice of having a separate bootstrap file.
@brettz9 browser specific feature, for node platform is non-reproducible.
Which browser-specific features? There is very little that can only work in the browser.
We don't polyfill the non-standard InternalError
in Node, and there are some differences in how canvases get serialized, but otherwise, I think we have polyfills for almost everything that work fine in both.
@septs : The underlying issue has been resolved and tests are now passing. ~Btw, I noticed you updated your local branch, but unfortunately I amended the commit after you updated, so I guess you will want to either delete your branch or reset it to the current branch.~ Nevermind, I think I did that, so I'll just reset your branch myself.
@dfahlander : This includes a breaking change, updating engines
to Node >= 10. I know you haven't had time these days to work on the library, but just wanted to confirm such a change was all right before I merged?
@dfahlander : Are you ok with requiring a maintained Node version (>=10)? Many projects have been doing so, including testing tools for which it becomes increasingly complex to downgrade in CI to ensure old versions still work.
@brettz9 added to presets.