-
Notifications
You must be signed in to change notification settings - Fork 225
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
chore: add min version in TAV matrix #3531
Conversation
… testing for contextManager=patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav fastify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav fastify 20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav fastify 10
this did not run anything since min node version set is higher than 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav unknown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav unknown
this fails because the package name is not in listed in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav
.ci/tav.json
Outdated
"@koa/router,koa-router", | ||
"handlebars,pug", | ||
"bluebird,got" | ||
{"name": "@apollo/server", "minVersion": 18 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for reviewer: to run less tests we add minVersion
to 18. This shall be updated with the proper versions if the PR is okay to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav express
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav express,fastify 20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav express,nonExistingPackage,fastify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A couple of nits is all.
.ci/tav.json
Outdated
{"name": "ws", "minVersion": 8 }, | ||
{"name": "@koa/router,koa-router", "minVersion": 8 }, | ||
{"name": "handlebars,pug", "minVersion": 8 }, | ||
{"name": "bluebird,got", "minVersion": 8 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, I guess with all these minVersion: 8
s, it means there won't be a large reduction in the number of jobs. Still probably worth keeping.
(Not part of this PR, but I wonder if we could consider dropping testing for versions of these packages that are, say, older than 2y and not of the latest major version. That might enable raising the minVersion
here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can leverage the multiple package option we have, here we are testing 2 packages in the same job
{"name": "bluebird,got", "minVersion": 8 }
This is actually what I want to do for @aws-sdk/client-*
to test all of them at once for each version. There might be other groups we could make perhaps @opentelemetry/*
or mysql
? Of course some test review is needed to see compatibility. Also I think the runTestFixtures
util introduced in the ESM support PR would come handy
we could consider dropping testing for versions of these packages that are, say, older than 2y
I agree and also I think it could be something to motivate users to update
Co-authored-by: Trent Mick <[email protected]>
Co-authored-by: Trent Mick <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/test tav fastify 20
Update the format of `tav.json` file and changed the workflow actions to have more control on how to generate the job matrix for testing. --------- Co-authored-by: Trent Mick <[email protected]>
Tweak
tav.json
file andtav.yml
andtav-command.yml
actions so we can specify which is the min version where the module should be tested. The matrix generated contains only one row where each element containsmodule node_version
in a single place.set to draft to test it and discuss with the team
Checklist