-
Notifications
You must be signed in to change notification settings - Fork 513
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
feat(add-tav-action): add test-all-versions as github action #778
feat(add-tav-action): add test-all-versions as github action #778
Conversation
|
it seems like you're filtering only the packages which have changed using some other action? Why not use the built-in lerna filtering functionality? |
Codecov Report
@@ Coverage Diff @@
## main #778 +/- ##
=======================================
Coverage 94.90% 94.90%
=======================================
Files 13 13
Lines 707 707
Branches 142 142
=======================================
Hits 671 671
Misses 36 36 |
Looks like the check fails |
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.
It looks like you don't install dependencies or build the packages. Are you sure it will work? Can you please make a small change to a package with a test-all-versions script so we can verify that it runs properly? Modifying the readme slightly should be enough.
with: | ||
fetch-depth: 0 | ||
- run: npm install test-all-versions | ||
- run: lerna run test-all-versions --since origin/main |
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.
nit
- run: lerna run test-all-versions --since origin/main | |
- run: lerna run test-all-versions --since origin/main | |
you need to install dependencies and build the project. You can see how this is done in the unit test workflow but usually you have to do |
the |
I'm not sure why it says mongo isn't started. @rauno56 is there some additional needed step for the tav script? |
Some tests need an service to be running in order to work: see "services" in unit-test.yml. |
Thanks for adding this. Instrumentations are patching the internal logic of packages that are out of our control. It means that if the original instrumented library author published a new version to npm which breaks something in the instrumentation, no automatic process will catch that until someone makes a change to the instrumentation packages, or when a user complains (probably after something breaks in his deployment due to the OTEL installation). This had happened before with At the opentelemetry-ext-js project we run test-all-versions on all packages every day with github actions scheduled-events, so if someone pushes a new version to npm which is not compatible with the current instrumentation, we get notified (in slack) after at most 24 hours. |
We could do both but i worry it may require too much bandwith to fix them when alert comes off (i guess it depend of component maintainers) resulting in maintainer generally ignoring errors |
I think not knowing about errors is a worse outcome than having errors that go unfixed for a while. I think this comes down to trusting the component authors and giving them a chance to succeed first before assuming they will fail. |
Lets do both then ? |
…ntelemetry-js-contrib into add-test-all-versions-action
…to add-test-all-versions-action
…trib into add-test-all-versions-action
…ntelemetry-js-contrib into add-test-all-versions-action
- run: npm install --ignore-scripts | ||
- run: lerna bootstrap --since origin/main --include-dependencies | ||
- run: lerna run compile --since origin/main --include-dependencies | ||
- run: lerna run test-all-versions --since origin/main --stream --concurrency=1 |
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.
nit:
- run: lerna run test-all-versions --since origin/main --stream --concurrency=1 | |
- run: lerna run test-all-versions --since origin/main --stream --concurrency=1 | |
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.
waiting for the cron scheduled workflow to be added before approving, otherwise seems good
@vmarchaud this particular action runs only the changed packages on pull requests. I think the full all-package cron can be done in a follow-up PR. @haddasbronfman do you agree? |
We discussed this privately on slack, i suggested to make only one for both (since it's closely related) but if you believe its bettter to make two i don't mind |
No I just didn't realize you had already discussed this. Go ahead with whatever you already decided. |
@haddasbronfman As you prefer then :) |
Hi, |
…ion (open-telemetry#778)" This reverts commit 77aefa6.
Which problem is this PR solving?
test-all-versions
run #756Short description of the changes
I added a new github action, which is using dorny/paths-filter to get all the changed packages, and for each such package, I run the command "npm run test-all-versions" - if it exist.