-
Notifications
You must be signed in to change notification settings - Fork 394
Add official system-tests workflow on pushes on master #4774
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
Conversation
Thank you for updating Change log entry section 👏 Visited at: 2025-07-02 10:07:54 UTC |
40277cc
to
259b86d
Compare
To add context to this PR: this is an experiment so we can do an apples-to-apples comparison with the existing integration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4774 +/- ##
==========================================
- Coverage 97.54% 97.52% -0.02%
==========================================
Files 1484 1483 -1
Lines 88499 88601 +102
Branches 4588 4592 +4
==========================================
+ Hits 86322 86412 +90
- Misses 2177 2189 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2025-07-08 15:00:51 Comparing candidate commit 5a8cc4f in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 43 metrics, 6 unstable metrics. |
f65ad6e
to
2a1c1b5
Compare
2a1c1b5
to
1a043b0
Compare
248f16e
to
6c2fb94
Compare
6c2fb94
to
216cc41
Compare
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.
Two questions:
-
Why does this workflow not run on pull requests, if we are attempting an "apples to apples" comparison?
-
Does the exsiting runner run "1600 weblog X scenario"?
And one more question: does this PR authenticate to docker hub, if so how, if not how is it not subject to rate limits? |
It does, the workflow inherits from your secrets, and if docker username/password are present, it do the authentification : https://github.com/DataDog/system-tests/blob/main/.github/workflows/run-end-to-end.yml#L107-L115 |
To prevent any issue on PRs, and being able to decide to use it on PR after few days of runs on
No, it run only 122 of them (details in PR description) |
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.
Oh, I must admit it was quite surprising to me that setting up this flow is ~130 lines of code -- for CI stuff I'm always fearing a bit of too much verbosity.
I don't see any issue, but I'll hold on pressing the approve button as this is a big change, so I think it makes sense to have a bit more time for the guild folks to chime in and put this through its paces.
Also, doesn't have to be in this PR, but I think once we agree to merge this, it makes sense to also remove the old integration (or at least disable it) so we don't re-run both sets of tests via both integrations.
Is it possible to add support for the forced-tests-list.json file ? I use it often, and at least @Strech and @marcotc have also used it. I can help if that's needed, I also did something similar for parametric-tests.yml, which uses system-tests official workflow |
Where would I find in the logs the confirmation that docker hub requests are authenticated? |
https://github.com/DataDog/dd-trace-rb/actions/runs/16024726999/job/45210078369#step:8:69 |
@vpellan Yes please :) ping me if you need any help |
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.
I was under the impression that this PR is the complete work, but if it's a partial change I have no objections.
Currently outstanding work items as I see them for the testing vs ST workflow to start:
- ST workflow needs to run on pull requests
- ST workflow needs to run all tests (including all of the ones presently being run by our workflow)
- ST workflow needs to support forced tests as mentioned by @vpellan
I'm curious about:
I thought it did? (I might be misunderstanding some part) |
I did not say the right thing and updated the comment: this PR claims to run all tests that our present workflow runs but the idea for changing to ST workflow was that it would run all tests so that we cannot have unintentionally un-executed tests. So the ST workflow should not require an enumeration of tests to run as this PR is presently doing. |
Got it, thanks for explaining. TBH I was the one that pushed for a more apples-to-apples comparison, as in our discussions so far we had a lot of (very fair) concerns about CI performance regressions with the changeover. So that one's on me! As you see in the current PR, it's more work to match our current set than it would be to run them all. That said, enabling all tests is definitely a trade-off, and does come with a clear CI time regression (+10 minutes). So my suggestion would be:
(To be clear: I really like speedy CI, but am happy to take the increased test coverage instead) If we decide we're happy, it seems trivial to have a follow-up to enable all the tests. |
There is also another point : @lloeki told on slack that it's useless to run the 1600 matrix point. As it's also a waste of ressource inside system-tests CI, I will define the set of useful matrix point inside system-tests. It means that if the guild chose to go with this official workflow, the definition will be way more simple - and still fast. |
What does this PR do?
Use the official system-tests WF for pushes on master branch to run end-to-end and parametric scenarios, to allow a comparizon with the current setup
Motivation
dd-trace-rb use a custom workflow which is hard to maintain
Comparizon
Notes
master
tracer-release
groups, rather hardcoding scenario namesChange log entry
None.
Additional Notes:
How to test the change?