Skip to content

Add test runs with minimum dependency versions #757

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cameel
Copy link
Member

@cameel cameel commented Jan 24, 2025

Depends on #756. Merged.

@cameel cameel force-pushed the testing-min-dependencies branch from 96304a9 to 1ea5caa Compare January 24, 2025 22:34
@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Jan 24, 2025
@coveralls
Copy link

coveralls commented Jan 24, 2025

Coverage Status

coverage: 84.537%. remained the same
when pulling f7e2bc9 on testing-min-dependencies
into f27395e on master.

@cameel cameel force-pushed the testing-min-dependencies branch 17 times, most recently from 449743f to 06fdfd0 Compare January 25, 2025 01:53
Comment on lines 19 to +24
- node-current:
run_coveralls: true
- node-current:
name: node-current-min-dependencies
min_dependencies: true
Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason CircleCI decided to change the name of the existing job to node-current-1. Not sure why, this does not happen with node-v12 for example. I tried adding name: node-current but it does not change this.

I guess we'll have to accept this and update the name of the required job in settings if we decide to merge the PR. I think we should make the min-dependencies jobs required as well.

Copy link
Member

@r0qs r0qs Mar 3, 2025

Choose a reason for hiding this comment

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

Isn't it because you have two node-current entries? I believe you may have accidentally duplicated the job name.

Copy link
Member Author

Choose a reason for hiding this comment

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

No. It has to be node-current for both because that's the name of the job that's being invoked in both cases:

node-current:
<<: *node-base
docker:
- image: cimg/node:current

The name of the run normally comes from that invoked job, but if it's invoked more than once, CircleCI adds numbers to disambiguate it. That's why there's the name parameter - note that it does not come from the job definition (see node-base). It's a built-in CircleCI thing meant for providing a custom name.

See also t_ext in Solidity CI config. We do it there the same way and it works.

@cameel cameel self-assigned this Jan 25, 2025
@cameel cameel requested a review from r0qs January 25, 2025 01:57
Copy link
Member Author

Choose a reason for hiding this comment

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

I wish npm had built-in support for this but apparently it does not so it has to be done on our own. I saw an old feature request for npm install --minimum, but it was eventually closed as stale.

@cameel cameel force-pushed the testing-min-dependencies branch from 06fdfd0 to 196f913 Compare January 25, 2025 03:46
@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Mar 5, 2025
@cameel cameel force-pushed the testing-min-dependencies branch from 196f913 to f7e2bc9 Compare March 5, 2025 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants