-
Notifications
You must be signed in to change notification settings - Fork 374
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
Changed forcing system-tests from env var in .yml to .json file #4092
Conversation
991fb94
to
a454b10
Compare
BenchmarksBenchmark execution time: 2024-11-13 14:48:29 Comparing candidate commit 42f9e92 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 28 metrics, 2 unstable metrics. scenario:tracing - Tracing.log_correlation
|
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 think the whole approach is correct 👍🏼 I would suggest to improve the naming for rest without good context of what's going on.
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.
Is this feature meant for releases only or also for development? I.e. if I am developing a system test that also requires changes in tracer, could I force that system test in the tracer branch?
Another comment I have is regarding the empty json file. My impression is that an empty file is not valid json payload, is that correct? Meaning the empty file wouldn't parse? If so I would suggest instead of the empty file, have forced-tests-list.json.sample
with sample contents (e.g. the same sample given in the documentation) which would 1) not have an unusable file in the tree and 2) would help someone craft the correct file.
This feature will make you able to force-execute a system-test on dd-trace-rb's Github CI without the need to create a PR on system-tests side. (which will just declare a version in the manifest, will not be visible on dd-trace-rb's CI results while the PR is not merged on system-tests main branch, and will break system-tests CI while the feature that is being worked on is not merged on dd-trace-rb's master branch) But this will only force them on dd-trace-rb CI, meaning that they will not be reported on the feature parity dashboard (or reported as easy-wins).
Yes an empty JSON file is not a valid JSON payload, which is why there is a condition for the file to not be empty before parsing it. But it should also work the same if we just put |
dc5dcf0
to
42f9e92
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4092 +/- ##
==========================================
- Coverage 97.77% 97.77% -0.01%
==========================================
Files 1350 1350
Lines 81062 81062
Branches 4085 4085
==========================================
- Hits 79259 79258 -1
- Misses 1803 1804 +1 ☔ View full report in Codecov by Sentry. |
What does this PR do?
This PR change the forcing of system-tests from environment variable in system-tests.yml to a .json file. This enables the forcing of system-tests on multiple scenarios at the same time. This will also be easier to clean in the post release steps of fast_castle.
Motivation:
fast_castle post release cleanup step currently contain bugs, and the current force-executing of tests feature is pretty limited.
Change log entry
None
Additional Notes:
Github Actions is very limited and I believe poorly documented, I'd really enjoy any advice on what/how it could be done better
How to test the change?
Add a test you want to force-execute in the .github/forced-tests-list.json file by following the documentation template