search

karma-runner / karma-jasmine

A Karma plugin - adapter for Jasmine testing framework.
MIT License
542 stars 162 forks source link

Update dependencies and drop Node.js 10 support #297

Closed XhmikosR closed 2 years ago

XhmikosR commented 2 years ago

Requires #296 so that we have CI running.

Fixes #303, fixes #304 along the way

TODO:

nicojs commented 2 years ago

@XhmikosR do you know who has maintainer access here? @johnjbarton isn't responding, would love to get some PR's in.

XhmikosR commented 2 years ago

Just be patient, it'll happen when people are back at their offices.

nicojs commented 2 years ago

I admire your optimism, but my PR #290 has been waiting for 6 months.

I'm not judging here, I know how it is. Just wondering who to contact 😌

XhmikosR commented 2 years ago

Well, I guess @jginsburgn ^^

XhmikosR commented 2 years ago

@jginsburgn I'm having trouble with commitlint, I could use a little help. That being said, commitlint should run once in a separate workflow and shouldn't block the other tests.

Also, should I drop Node.js 10 support here? It's causing issues AFAICT

sgravrock commented 2 years ago

Also, should I drop Node.js 10 support here? It's causing issues AFAICT

I'm not sure how much bearing this has on what karma-jasmine does, but FWIW jasmine 4.x requires at least Node 12.17. Older versions can mostly be made to work with a configuration change, at least for now, but we no longer test against them and aren't making any attempt to keep them working.

jginsburgn commented 2 years ago

Also, should I drop Node.js 10 support here? It's causing issues AFAICT

I'm not sure how much bearing this has on what karma-jasmine does, but FWIW jasmine 4.x requires at least Node 12.17. Older versions can mostly be made to work with a configuration change, at least for now, but we no longer test against them and aren't making any attempt to keep them working.

TBH I think it should trigger a breaking change. @XhmikosR, @sgravrock and @devoto13 WDYT?

jginsburgn commented 2 years ago

Also, we should probably support Node.js 14, 16 and 18.

devoto13 commented 2 years ago

Yeah, I think that bumping the karma version we test against, major Jasmine bump and minimum Node version bump should trigger a major release.

jginsburgn commented 2 years ago

Let's merge this PR once https://github.com/karma-runner/karma-jasmine/issues/312 gets settled.

jginsburgn commented 2 years ago

FYI https://github.com/karma-runner/karma-jasmine/issues/312 is still blocking this! I will try to resolve it asap.

XhmikosR commented 2 years ago

Rebased and updated to the latest deps.

Personally I would just release this as a major version bump and stop worrying about it :)

XhmikosR commented 2 years ago

BTW how am I supposed to construct the commit message so that linting is passing?

jginsburgn commented 2 years ago

Rebased and updated to the latest deps.

Personally I would just release this as a major version bump and stop worrying about it :)

I agree with you for this release, as latest is already stale. But at least we should couple it with https://github.com/karma-runner/karma-jasmine/pull/304 and all the dependency upgrades we can do! WDYT @devoto13 ?

jginsburgn commented 2 years ago

BTW how am I supposed to construct the commit message so that linting is passing?

We are using Angular's Commit Message Format, which for breaking changes instead of using the exclamation point after the commit type, uses a special kind of footer.

devoto13 commented 2 years ago

I agree with you for this release, as latest is already stale. But at least we should couple it with https://github.com/karma-runner/karma-jasmine/pull/304 and all the dependency upgrades we can do! WDYT @devoto13 ?

@jginsburgn I think @XhmikosR is right and we don't need to wait for the beta branch here. I've updated the PR and added BREAKING CHANGE section to each commit, so the changelog should look good now. We can merge this PR, release 5.0.0, and do any other cleanups/updates in the follow-up PRs.

XhmikosR commented 2 years ago

Rebased on last time. I'm still having trouble with the Angular commit messages, so if the commit messages need fixing, feel free to reword :)

Should be ready to land from my side.

devoto13 commented 2 years ago

@XhmikosR Well, I did just that before your last rebase 😉

You can set commit message to be the same as in https://github.com/karma-runner/karma-jasmine/commit/1d00309ac0ca7913492d59639f91485c94771857, https://github.com/karma-runner/karma-jasmine/commit/17934c1b224e8505d3675cb4ac66595cf790a60e and https://github.com/karma-runner/karma-jasmine/commit/2aa3e818f8c5f1974a042efa1cc5ca593cd87fc3.

XhmikosR commented 2 years ago

@devoto13 thanks, and sorry about that. I missed the force push, I only saw the approval :/

karmarunnerbot commented 2 years ago

:tada: This PR is included in version 5.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

XhmikosR commented 2 years ago

Something else I noticed @jginsburgn @devoto13: should we move jasmine-core to peer dependencies? In major version bump, ofc.

jginsburgn commented 2 years ago

I think it makes sense to have it as a dependency! IIUC peer dependencies are meant to be used by plugins to indicate the host package.

XhmikosR commented 2 years ago

Yeah, I'm in between myself... I just noticed that other packages use jasmine-core as a peer dependency. Not a big deal since as long as we are up to date package managers can deduplicate the package.

devoto13 commented 2 years ago

I agree with Jonathan, it's not a plugin and I don't see a good reason why a project must use a single instance of the jasmine-core, so I would prefer to keep it as a regular dependency to make karma-jasmine easier to install.

jginsburgn commented 2 years ago

Then it is settled: let's keep it as a dependency :)