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.

Sign up for GitHub

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

Jump to bottom

add one second test check to CI #1033

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

add one second test check to CI #1033

wants to merge 6 commits into from
+51 −3

Conversation

stan-dot
Copy link
Contributor

@stan-dot stan-dot commented Feb 3, 2025

Fixes #1003

Instructions to reviewer on how to test:

  1. Check the tests
  2. Confirm thing print("✅ All tests ran within the acceptable time limit.") or print("❌ The following tests exceeded the 1s threshold:") happened

Checks for reviewer

  • Would the PR title make sense to a scientist on a set of release notes
  • If a new device has been added does it follow the standards
  • If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
  • Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

@stan-dot stan-dot added enhancement New feature or request github_actions Pull requests that update GitHub Actions code low priority Not needed for production in the near future Developer Experience labels Feb 3, 2025
@stan-dot stan-dot self-assigned this Feb 3, 2025
@stan-dot stan-dot linked an issue Feb 3, 2025 that may be closed by this pull request
Add CI job that fails PRs if tests are taking >1s #1003
Open
@stan-dot stan-dot force-pushed the 1003-add-ci-job-that-fails-prs-if-tests-are-taking-1s branch from d3e68ee to deed495 Compare February 4, 2025 10:48
@stan-dot stan-dot requested a review from DominicOram February 4, 2025 10:51
@stan-dot stan-dot marked this pull request as ready for review February 4, 2025 10:52
@stan-dot stan-dot requested a review from a team as a code owner February 4, 2025 10:52
DominicOram
DominicOram requested changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you. I think once we merge into main it will pass again as I moved the CLI test in #1000. Only issue is that it now looks like the tests are being run twice? The logs are printing results twice and if you compare the time on https://github.com/DiamondLightSource/dodal/actions/runs/13132250037/job/36639728998 to https://github.com/DiamondLightSource/dodal/actions/runs/13133823256/job/36644639445?pr=1033 its ~2x the time.

@stan-dot
Copy link
Contributor Author

stan-dot commented Feb 4, 2025 •
edited
Loading

looking into it now

update: noticed where is this happening

@stan-dot stan-dot force-pushed the 1003-add-ci-job-that-fails-prs-if-tests-are-taking-1s branch from 519fb11 to 8535a08 Compare February 4, 2025 12:07
@stan-dot
Copy link
Contributor Author

stan-dot commented Feb 4, 2025 •
edited
Loading

reproducing lcoally:

 1027 passed, 4 skipped, 40 deselected in 31.79s ==================================================================
_____________________________________________________________________________________ summary ______________________________________________________________________________________
  report: commands succeeded
  congratulations :)
  
================================================================= 1027 passed, 4 skipped, 40 deselected in 31.28s ==================================================================
_____________________________________________________________________________________ summary ______________________________________________________________________________________
tests: commands succeeded
congratulations :)

beginning to suspect that the culprit is pytest that runs twice due to a request to have both coverage and report json for time
---------- coverage: platform linux, python 3.10.16-final-0 ----------

this line only occurs once in this run https://github.com/DiamondLightSource/dodal/actions/runs/13135068486/job/36648559977?pr=1033

reading the docs...
https://pytest-cov.readthedocs.io/en/latest/reporting.html

@stan-dot
Copy link
Contributor Author

stan-dot commented Feb 4, 2025

tried to replicate time doubling on a smaller scale and the undesired doubling does not happen

@stan-dot
Copy link
Contributor Author

stan-dot commented Feb 4, 2025 

❌ The following tests exceeded the 1s threshold:
 - system_tests/test_cli.py::test_cli_version: 3.46s

should I exclude this?

@stan-dot
Copy link
Contributor Author

stan-dot commented Feb 4, 2025 •
edited
Loading

it's possible that addition of dev-dependency pytest-json-report is not good and a json report could be generated solely from pytest-cov

update: the cov.json does not cover time

therefore the pytest-json-report is good actually

@DominicOram
Copy link
Contributor

should I exclude this?

No, as I said above, it will be fixed if you merge in main. Happy to discuss this issue further but with so many comments above it's hard for me to know if I'm getting a notification that requires action or not. It would be helpful to me if you could summarise findings into one comment once you have solved the issue or need input from someone else, thanks!

@stan-dot
Copy link
Contributor Author

stan-dot commented Feb 4, 2025

I thought that I had already merged in main at the moment of making that comment

@stan-dot stan-dot force-pushed the 1003-add-ci-job-that-fails-prs-if-tests-are-taking-1s branch from 90c7aa7 to 229f472 Compare February 4, 2025 15:56
@DominicOram
Copy link
Contributor

I thought that I had already merged in main at the moment of making that comment

Ah, my bad. I think that the solution is that we need different thresholds for system tests vs unit tests. Can we set like a 5s limit on system tests or we can just ignore if they go over the duration, I don't mind which.

@stan-dot stan-dot assigned stan-dot and unassigned stan-dot Feb 4, 2025
@stan-dot
Copy link
Contributor Author

stan-dot commented Feb 4, 2025

Can we set like a 5s limit on system tests or we can just ignore if they go over the duration, I don't mind which.

making it 5 s for system tests

@codecov Codecov
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.58%. Comparing base (f69b1e6) to head (01664bd).

Additional details and impacted files 
@@             Coverage Diff             @@
##             main    #1033       +/-   ##
===========================================
- Coverage   97.62%   28.58%   -69.05%     
===========================================
  Files         159      143       -16     
  Lines        6581     6069      -512     
===========================================
- Hits         6425     1735     -4690     
- Misses        156     4334     +4178

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stan-dot
Copy link
Contributor Author

stan-dot commented Feb 4, 2025

this seems to work now @DominicOram

@stan-dot
Copy link
Contributor Author

stan-dot commented Feb 4, 2025

coverage took a nosedive though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DominicOram DominicOram DominicOram requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@stan-dot stan-dot

Labels
Developer Experience enhancement New feature or request github_actions Pull requests that update GitHub Actions code low priority Not needed for production in the near future
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add CI job that fails PRs if tests are taking >1s
2 participants
@stan-dot @DominicOram