Skip to content
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

Force CI to run on every PR #286

Merged
merged 14 commits into from
Jun 1, 2023
Merged

Force CI to run on every PR #286

merged 14 commits into from
Jun 1, 2023

Conversation

cjbarth
Copy link
Contributor

@cjbarth cjbarth commented May 29, 2023

To improve code quality going forward, this PR will enable all tests to run on every PR.

In order to get tests to most closely represent an actual deployment npm ci is used, this, however, isn't available on older versions of Node, so the test matrix doesn't reflect the actually supported versions of Node. We should do a semver-major change to get this version up to Node@8, at least, to be able to test all supported versions.

@cjbarth cjbarth added the chore label May 29, 2023
@LoneRifle
Copy link
Collaborator

Could you clarify what you mean by broken tests? I just did npm test on my local dev and on a gitpod instance using Node 16 as well as Node 4, and all the tests pass.

@cjbarth
Copy link
Contributor Author

cjbarth commented May 30, 2023

This is why we have code review :)

It turns out that nodeunit doesn't do globbing very well. Once I reverted the glob, all the tests started working. Sorry about that.

So, the only thing that this PR would do then is to make sure all tests run on every PR.

@cjbarth cjbarth changed the title Disable broken tests; force CI to run on every PR Force CI to run on every PR May 30, 2023
LoneRifle
LoneRifle previously approved these changes May 31, 2023
Copy link
Collaborator

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally lgtm. Some concerns about the nature of the changes but non-blockers.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@shunkica
Copy link
Contributor

It turns out that nodeunit doesn't do globbing very well. Once I reverted the glob, all the tests started working. Sorry about that.

Maybe this is an opportunity to move away from nodeunit as it is a dead project.

@cjbarth
Copy link
Contributor Author

cjbarth commented May 31, 2023

@shunkica , I'd love to see a PR that does that. We currently use Jasmine in the other projects here, so it would be reasonable to move to that for consistency. However, anything would be better than nodeunit at this point.

@cjbarth
Copy link
Contributor Author

cjbarth commented May 31, 2023

@shunkica , would you be willing to put up a PR for a migration of the tests?

@cjbarth
Copy link
Contributor Author

cjbarth commented May 31, 2023

@LoneRifle , I think I've addressed all your concerns. Feel free to have another look at this. Your feedback is quite good. I look forward implementing some of this over in node-saml and passport-saml.

Copy link
Collaborator

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, as discussed. Thanks for looking into this!

@cjbarth cjbarth merged commit 1001d12 into master Jun 1, 2023
@cjbarth cjbarth deleted the ci-tests branch June 1, 2023 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants