Get tests and builds working in current Node.js versions #195
Conversation
This is the latest v1.x release, and allows Jasmine browser tests to work in modern Node.js releases (Node.js v12+). The latest version is actually v4, but we can't upgrade to v2+ because there is no compatible version of grunt-template-jasmine-istanbul (top-of-tree there is compatible, but was never released and apparently has unaddressed test failures; developed appears to be stopped, so we also can't expect a future release).
This is the latest version and allows grunt-jasmine-node to function in modern versions of Node.js. Unfortunately, it contains a *huge* regression in configuration and requires this super-wonky hack. Some folks in grunt-jasmine-node's issues have proposed other ways to work around this in how you structure your config, but none of those actually work correctly (I did a lot of testing here!).
Using Jasmine directly is pretty simple (moreso than the previous hacks to use grunt-jasmine-node!) and reduces a bunch of compatibility and security issues. It also lets us use the same version of Jasmine for Node.js and browser tests.
…to v0.6 In order to a) remove a critical code injection vulnerability and b) upgrade browser tests to a recent version of Headless Chrome (instead of PhantomJS), this upgrades grunt-contrib-jasmine to v4.0.0 and grunt-template-jasmine-istanbul to v0.6.0 (via git; this version was never published on NPM). Upgrading the former requires upgrading the latter. Important notes: 1. This is a little iffy, in that this version of grunt-template-jasmine-istanbul was never published to NPM, supposedly on account of it causing some test failures (unclear whether those failures are problems with the code or with the tests): maenu/grunt-template-jasmine-istanbul#64 (comment) 2. This requires installation via `npm install --legacy-peer-deps` because this version of grunt-template-jasmine-istanbul has a peer dependency on grunt-contrib-jasmine v2.x, but we are now on v4.x. It still works fine, but is a little messy to require this extra flag (this should only affect installs that include dev dependencies, not people who are installing loglevel as a dependency of their project). The best solution to these is probably to drop grunt-template-jasmine-istanbul altogether -- we aren't really using the coverage information it gathers right now. The next alternative is to fork and vendor it like we did for grunt-template-jasmine-requirejs.
| [vendor/grunt-template-jasmine-requirejs/**/*] | ||
| indent_size = 2 |
There was a problem hiding this comment.
I probably should have done this when I vendored grunt-template-jasmine-requirejs. It was helpful when I had to make changes. 🤷
.github/workflows/ci.yml
Outdated
| cache: npm | ||
| cache-dependency-path: 'package.json' | ||
|
|
||
| - name: Install dependencies | ||
| run: npm install | ||
| run: npm install --legacy-peer-deps |
There was a problem hiding this comment.
The --legacy-peer-deps option is required for grunt-template-jasmine-istanbul v0.6.0, since it depends on grunt-contrib-jasmine v2.x and we are now on v4.x. It’s less than ideal, but is only needed in order to install dev dependencies; a consumer of loglevel should not need it when running npm install loglevel in their package.
As noted in the PR description, I actually think the best solution if we want to remove this is to drop grunt-template-jasmine-istanbul altogether.
There was a problem hiding this comment.
This sounds like something that would make contribution difficult though, it would be nice if this worked out of the box so that contributors can just run npm install without problems.
You can run npm config --location=project set legacy-peer-deps=true to fix this - that'll create a .npmrc that will enable this setting by default for any npm command run in this repo, and then you can drop that option from here.
(But as noted in my other comment - I think we should just drop this instead, so you don't actually need to do that unless there's some other legacy peer deps issue elsewhere too)
There was a problem hiding this comment.
Ah, I had not thought about adding a .npmrc file for this, good catch! But also, yeah, agree on the downsides anyway.
In 72182e5, I upgraded grunt-contrib-jasmine to v4, and forgot this upgraded Jasmine (for browser tests) to v5.1. This upgrades the Jasmine version used for Node.js tests to match.
I noticed this while checking the Jasmine version, now that I'm running in a local browser. If testing in an environment where no cookies are set *yet*, the tests assume cookies aren't supported, which is not correct. This fixes the issue.
Forgot to check Node.js support here. Jasmine 5.1 requires Node.js 18+. Even though we've decided we aren't worrying about requiring newer versions now, this seems a bit too big of a jump. Setting aside for now, but we can easily bring this back if we want.
|
Realized today that in 72182e5 when I updated the version of grunt-contrib-jasmine to v4, it also upgrade Jasmine to v5.1 for browser tests. I figured I should upgrade the Node.js tests to match, which was pretty simple, but then realized those require at least Node 18+, which seemed like a bit too big of a jump, so I reversed that. This is easy to put back in with a single Along the way, I also noticed that cookie support checking in the tests is not actually working correctly, resulting in some tests getting skipped when they shouldn't be (this was fine in PhantomJS since it doesn’t properly support cookies, but different in headless Chrome or in live tests with |
Yeah, I'm fine with that. It seems like the current approach adds complexity (requiring legacy peer deps options, using unofficial releases, depending on a now-unmaintained dead project) and as we discussed in #191 we're not actually using the coverage data now. If it was easy to keep working that'd be fine, but this seems more hassle than its worth, so I'm happy to cut it entirely and streamline. |
|
Makes sense. That should be done, and this PR should be ready to go. |
This gets tests and builds working in current versions of Node.js, from v6 through v20 (current active release), and in MacOS on Apple Silicon. The main goal here is to make it possible to run tests and builds on most types of computers without lots of complex setup, so it’s not too hard for contributors to test and validate their work.
The main changes here are:
Upgrade grunt-contrib-jasmine to v4.0.0 and grunt-template-jasmine-istanbul to v0.6.0 (via git, not NPM). Unfortunately, grunt-template-jasmine-istanbul now points to the git commit for v0.6.0 instead of NPM, because this version was never published on NPM. I believe that is because of some potential issues with tests in the final change (it looks like nobody ever verified whether actually are issues — the author supposes they might be resolved when another, parallel PR merges, and that PR did in fact get merged).
Some options on making this situation better:
Remove grunt-jasmine-node in favor of using jasmine directly. The new, inline Grunt task for calling Jasmine directly is extremely simple, and I think it poses low maintenance overhead. It also removes some security issues and allows us to use the same version of Jasmine in both Browser-based tests and Node.js-based tests.
An alternative here is to use grunt-jasmine-node v0.3.1, but it requires a ridiculous hack because it turns out to be totally broken with respect to loading configuration options. More info in this commit that tries using it: 94acb4e. All versions of grunt-jasmine-node that work in modern Node.js versions have this problem. I would definitely recommend against this.
This completes the first two items from #191 (comment).