-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run browser tests in browser #23
Comments
lucaswerkmeister
added a commit
that referenced
this issue
Sep 18, 2022
Apparently mocha-headless doesn’t work with newer Chrome/Chromium versions – it broke on my local system a while ago, and eventually also in CI. I haven’t been able to find a working replacement (Karma looked most promising, but seems to be married to RequireJS too tightly to support modules as test files), and in the meantime Node released experimental fetch() support – it’s not good enough to replace the axios backend (no cookie support), but enough to run the browser tests. So for now, we run the “browser” tests under Node as well, at least in CI and when you run `npm t`; but also add a browser-test.html file that can be loaded manually in a browser (using `python -m http.server` or equivalent – it won’t work using the file:// protocol), and tweak browser.test.js so that the Chai import works in Node and browser. This requires bumping the test-full CI job to Node 18; I’m generally happy to do that, since test-most (Node 12) still checks support for older Node versions, but at some point Node removed the --harmony-top-level-await option, so we need to tweak the test:readme command to retry running Node without it. (Fortunately we don’t have to repeat the sed command, when Node complains about the invalid option it doesn’t consume any stdin.) I hope to eventually replace this with proper testing in a browser again, see #23.
lucaswerkmeister
added a commit
that referenced
this issue
Sep 18, 2022
Node versions between 8.5 (inclusive) and 16 (exclusive) have a performance object, but it’s only available for import from the node:perf_hooks internal package, not on the global scope where we want to use it. Add a file that imports performance from the package and then adds it to the global scope; that has an impact beyond this library, but IMHO that’s acceptable, since Node 16 already decided that it was okay to add the object to the global scope. We load this file in node.js and during the unit tests. If anyone uses axios.js directly instead of via node.js, they’re responsible for loading the file themselves. In the Node integration tests, we load the file via node.js; in the Browser integration tests, which we now also run in Node (see #23), we don’t load the file, but we also don’t depend on retry behavior, so it should be fine. I’m not generally opposed to finding another solution for this, but for now this fixes #1.
lucaswerkmeister
added a commit
that referenced
this issue
Sep 21, 2022
Disable some new warnings they show for browser.test.js – I know this code is ugly and have an open task for fixing it (#23), but for now it has to be like that. Remove an unused eslint-disable in add-performance-global.js – not sure why eslint used to complain about that.
mochify.js might be an option to look into. |
Apparently they’re currently undergoing a rewrite; the rewritten version is supposed to support ES modules, but the current one doesn’t, so we can’t use it yet. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We used to run
test/integration/browser.test.js
using mocha-headless, but that broke (and there’s no clear way to report it, since that package has no source code repository or issue tracker). I’m about to push a commit that, for now, runs the test in Node instead, using Node’s experimentalfetch()
support, to unbreak the tests; it would be nice to somehow run them in a browser again, though.The text was updated successfully, but these errors were encountered: