Skip to content
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

ci: error out if no riot hashes are found when running test suites #7051

Merged
merged 15 commits into from
Sep 27, 2023

Conversation

romainkomorndatadog
Copy link
Collaborator

@romainkomorndatadog romainkomorndatadog commented Sep 26, 2023

If no riot hashes are found for some reason, scripts/run-test-suite should error and exit.

This also adds a bit of logging for fun.

Example of failure due to no matching hashes:

root@docker-desktop:~/project# scripts/run-test-suite romainlikethelettucewithoutthee
No riot hashes found for pattern: romainlikethelettucewithoutthee

Example of run with matching hashes:

root@docker-desktop:~/project# scripts/run-test-suite ci_visibility
Found 6 riot hashes.
Running riot hash: 12a04f4
INFO:riot.riot:Generating virtual environments for interpreters Interpreter(_hint='3.12')
...

Checklist

  • Change(s) are motivated and described in the PR description.
  • Testing strategy is described if automated tests are not included in the PR.
  • Risk is outlined (performance impact, potential for breakage, maintainability, etc).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Library release note guidelines are followed. If no release note is required, add label changelog/no-changelog.
  • Documentation is included (in-code, generated user docs, public corp docs).
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Title is accurate.
  • No unnecessary changes are introduced.
  • Description motivates each change.
  • Avoids breaking API changes unless absolutely necessary.
  • Testing strategy adequately addresses listed risk(s).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Release note makes sense to a user of the library.
  • Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment.
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy
  • If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

@romainkomorndatadog romainkomorndatadog added the changelog/no-changelog A changelog entry is not required for this PR. label Sep 26, 2023
@romainkomorndatadog romainkomorndatadog self-assigned this Sep 26, 2023
@romainkomorndatadog romainkomorndatadog requested a review from a team as a code owner September 26, 2023 14:28
emmettbutler
emmettbutler previously approved these changes Sep 26, 2023
mabdinur
mabdinur previously approved these changes Sep 26, 2023
@pr-commenter
Copy link

pr-commenter bot commented Sep 26, 2023

Benchmarks

Benchmark execution time: 2023-09-27 14:53:09

Comparing candidate commit 1c1780b in PR branch romain.komorn/ci_error_if_no_hashes with baseline commit f00acf8 in branch 2.x.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 90 metrics, 0 unstable metrics.

@romainkomorndatadog romainkomorndatadog requested a review from a team as a code owner September 27, 2023 07:54
P403n1x87
P403n1x87 previously approved these changes Sep 27, 2023
@P403n1x87
Copy link
Contributor

Any value in backporting this?

@romainkomorndatadog
Copy link
Collaborator Author

I'm disabling auto-merge because I don't trust this (because of the ddtracerun test flaking on me).

I'm also actually not sure how many PRs we'll end up blocking if this merges.

@P403n1x87 , as far as backporting this... yeah it's probably be good to backport into 1.20 and 2.0, at least.

@romainkomorndatadog
Copy link
Collaborator Author

Hm, I think this falls down if there are more parallel runs than environments. 6 out of the 8 envs run because we only find 6 hashes.

Put up a fix in my last commit, looks nicer (in that it doesn't crash) but the parallel runs seem a bit pointless:

Run with hashes:
image

Run that has no hashes:
https://app.circleci.com/pipelines/github/DataDog/dd-trace-py/46740/workflows/0ee091f5-e622-41e8-bdb1-3dee7191a13d/jobs/3036375/parallel-runs/6?filterBy=ALL
image

@romainkomorndatadog
Copy link
Collaborator Author

@romainkomorndatadog
Copy link
Collaborator Author

@emmettbutler , think this is good to go. I'm a bit hesitant about switching to using an array since we don't really use any in the rest of the script (or in our scripts, usually), but I like being able to give counts for some reason. The other options for counts was something like echo $VAR | wc -w or but that felt "bad" for some reason.

@emmettbutler emmettbutler enabled auto-merge (squash) September 27, 2023 13:14
auto-merge was automatically disabled September 27, 2023 15:31

Pull request was closed

@juanjux juanjux merged commit c7b2520 into 2.x Sep 27, 2023
26 checks passed
@juanjux juanjux deleted the romain.komorn/ci_error_if_no_hashes branch September 27, 2023 15:59
@github-actions github-actions bot added this to the v2.1.0 milestone Sep 27, 2023
tabgok pushed a commit that referenced this pull request Sep 28, 2023
…7051)

If no riot hashes are found for some reason, `scripts/run-test-suite`
should error and exit.

This also adds a bit of logging for fun.

Example of failure due to no matching hashes:
```
root@docker-desktop:~/project# scripts/run-test-suite romainlikethelettucewithoutthee
No riot hashes found for pattern: romainlikethelettucewithoutthee
```

Example of run with matching hashes:
```
root@docker-desktop:~/project# scripts/run-test-suite ci_visibility
Found 6 riot hashes.
Running riot hash: 12a04f4
INFO:riot.riot:Generating virtual environments for interpreters Interpreter(_hint='3.12')
...
```

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants