Add check for slow tests to CI

[stan-dot]

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}

[DominicOram]

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]

looking into it now

update: noticed where is this happening

[stan-dot]

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]

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

[stan-dot]

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

should I exclude this?

[stan-dot]

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]

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]

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

[DominicOram]

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]

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 Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.69%. Comparing base (81496ff) to head (04b7aa5).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1033   +/-   ##
=======================================
  Coverage   97.69%   97.69%           
=======================================
  Files         160      160           
  Lines        6629     6629           
=======================================
  Hits         6476     6476           
  Misses        153      153           

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

[stan-dot]

this seems to work now @DominicOram

[stan-dot]

coverage took a nosedive though

[DominicOram]

this seems to work now @DominicOram
coverage took a nosedive though

Similar to above, it's not clear to me from these comments what you feel the next steps are on this? Are you still looking at the coverage issue? Would you like help on it? Do you want me to merge it despite the coverage issue?

[stan-dot]

These are files that didn't have author revisions, but contain unexpected coverage changes

I haven't seen this before, investigating

https://docs.codecov.com/docs/unexpected-coverage-changes

https://app.codecov.io/gh/DiamondLightSource/dodal/pull/1033/indirect-changes#f0948950da8d813eb6b190daf1763a51-R2

[stan-dot]

it's not clear to me from these comments what you feel the next steps are on this? Are you still looking at the coverage issue?

yes I have no idea why is the coverage going down. I have never seen this before. Presumably it'd be good if someone took a look at it too.

[stan-dot]

atm the CI fail is due to test time:

Run python .github/scripts/check_test_durations.py unit-report.json 1
❌ The following tests exceeded the 1.00s threshold:

• tests/devices/unit_tests/test_apple2_undulator.py::test_given_gate_never_closes_then_setting_gaps_times_out: 3.01s
• tests/devices/unit_tests/test_apple2_undulator.py::test_given_gate_never_closes_then_setting_phases_times_out: 1.02s
• tests/devices/unit_tests/test_apple2_undulator.py::test_given_gate_never_closes_then_setting_jaw_phases_times_out: 1.01s

[Relm-Arrowny]

atm the CI fail is due to test time:

Run python .github/scripts/check_test_durations.py unit-report.json 1 ❌ The following tests exceeded the 1.00s threshold:

• tests/devices/unit_tests/test_apple2_undulator.py::test_given_gate_never_closes_then_setting_gaps_times_out: 3.01s
• tests/devices/unit_tests/test_apple2_undulator.py::test_given_gate_never_closes_then_setting_phases_times_out: 1.02s
• tests/devices/unit_tests/test_apple2_undulator.py::test_given_gate_never_closes_then_setting_jaw_phases_times_out: 1.01s

I can have a look I am pretty sure they run close to 1 sec but I am not sure about the 3.01s.

[Relm-Arrowny]

I can have a look I am pretty sure they run close to 1 sec but I am not sure about the 3.01s.

yes the main version definitely took 3 second and it will be fix with #1032.

[DominicOram]

add timeout = 1 in tool.pytest.ini_options ?

Yh, I don't mind solving it like this. But I guess it would be annoying if this meant that:

• No subsequent tests are run after the timeout one
• You don't get to see if the timeout test failed for other reasons

But I think we can live with these things :)

Presumably it'd be good if someone took a look at it too.

Ok, I will try and find some time to take a look

[stan-dot]

============================================= 1050/1090 tests collected (40 deselected) in 5.48s =============================================

for

pytest -m 'not (s03 or adsim)' --collect-only

while>>
for system_tests =============================================== 2/42 tests collected (40 deselected) in 3.43s ================================================

and tests: ======================================================= 1048 tests collected in 4.02s ========================================================

so the same number of tests is collected.

[stan-dot]

waiting for #1032 to be fixed

[stan-dot]

@Relm-Arrowny how is that other PR going?

#1033 (comment)

[DominicOram]

@Relm-Arrowny how is that other PR going?

#1033 (comment)

This is on me to review. I will do so today

[Relm-Arrowny]

add timeout = 1 in tool.pytest.ini_options ?

Yh, I don't mind solving it like this. But I guess it would be annoying if this meant that:

• No subsequent tests are run after the timeout one
• You don't get to see if the timeout test failed for other reasons

I have not tested it lately but I am 80% sure it run all the test even if it time out and the test will run to completion. The time out just fail it after.

[DominicOram]

Great, thank you! Passing now that the long tests have been fixed