feat(add-tav-action): add test-all-versions as github action

[haddasbronfman]

Which problem is this PR solving?

• This PR is solving this issue: Run a daily test-all-versions run #756

Short 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.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ haddasbronfman (1e2ca4b)

[dyladan]

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]

Codecov Report

Merging #778 (a32ffad) into main (b6110c6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #778   +/-   ##
=======================================
  Coverage   94.90%   94.90%           
=======================================
  Files          13       13           
  Lines         707      707           
  Branches      142      142           
=======================================
  Hits          671      671           
  Misses         36       36           

[dyladan]

Looks like the check fails

[dyladan]

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.

[dyladan]

nit

Suggested change

	- run: lerna run test-all-versions --since origin/main
	- run: lerna run test-all-versions --since origin/main

[dyladan]

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 npm install --ignore-scripts, then lerna bootstrap --since origin/main --include-dependencies, then lerna run compile --since origin/main --include-dependencies. Then you should be able to do the test all versions script.

[dyladan]

the feat identifier in the title signifies the minor version number to be incremented in our release automation. Please use a more appropriate conventional commits identifier like test

[dyladan]

I'm not sure why it says mongo isn't started. @rauno56 is there some additional needed step for the tav script?

[rauno56]

Some tests need an service to be running in order to work: see "services" in unit-test.yml.

[blumamir]

Thanks for adding this.
The current action will run only against packages that had changes in each PR.
This will do a good job of catching errors with old versions that are introduced in the current PR.

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 aws-sdk, which has a complex internal logic and tends to publish new versions often.

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.
Github action configuration for reference: https://github.com/aspecto-io/opentelemetry-ext-js/blob/master/.github/workflows/daily-test.yml
sidenote: the daily tests currently fails due to kafkajs, we need to look at it and fix it

[vmarchaud]

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

[dyladan]

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.

[vmarchaud]

Lets do both then ?

[dyladan]

nit:

Suggested change

	- run: lerna run test-all-versions --since origin/main --stream --concurrency=1
	- run: lerna run test-all-versions --since origin/main --stream --concurrency=1

[vmarchaud]

waiting for the cron scheduled workflow to be added before approving, otherwise seems good

[dyladan]

@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?

[vmarchaud]

@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

[dyladan]

@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.

[vmarchaud]

@haddasbronfman As you prefer then :)

[haddasbronfman]

Hi,
I prefer to do it on another PR is this is ok with you.