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

Fix flakey bart robot unit test #943

Open
olliesilvester opened this issue Dec 5, 2024 · 7 comments
Open

Fix flakey bart robot unit test #943

olliesilvester opened this issue Dec 5, 2024 · 7 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@olliesilvester
Copy link
Collaborator

One of our tests for the bart robot occasionally fails. We should fix this

tests/devices/unit_tests/test_bart_robot.py::test_given_program_not_running_and_pin_unmounts_then_mounts_when_load_pin_then_pin_loaded - AssertionError: Expected 'trigger' to have been called once. Called 0 times.

Or see https://github.com/DiamondLightSource/dodal/actions/runs/12165707719/job/33930319285

Acceptance Criteria

  • Test made more robust
@olliesilvester olliesilvester added bug Something isn't working good first issue Good for newcomers labels Dec 5, 2024
@paulorochaoliveira
Copy link

FYI, I'm taking a look at this flaky test right now.

@paulorochaoliveira
Copy link

It seems like what makes this test flaky is the sleep time sometimes being to short. Increasing the sleep time would most likely fix the issue.
Another more robust solution is to replace arbitrary sleeps with an event-driven approach. For example, use asyncio.Event to synchronize the test with the actual behavior.
What do you think @olliesilvester ?

@olliesilvester
Copy link
Collaborator Author

I think increasing the sleep is good enough for this issue, however I like the idea of using asyncio.Event instead. I will make an issue to replace all of our arbitrary sleeps in tests instead of this! Thanks!

@paulorochaoliveira
Copy link

Should I open a PR to at least close this issue?

@olliesilvester
Copy link
Collaborator Author

Yes, go ahead, thank you

arikaran-13 added a commit to arikaran-13/dodal that referenced this issue Dec 5, 2024
DiamondLightSource#943 Fix flakey bart robot unit test
arikaran-13 added a commit to arikaran-13/dodal that referenced this issue Dec 5, 2024
DiamondLightSource#943 load timeout and sleep time updated
paulorochaoliveira added a commit to paulorochaoliveira/dodal that referenced this issue Dec 5, 2024
olliesilvester pushed a commit that referenced this issue Dec 10, 2024
#943 Fix flakey bart robot unit test
huw-dls pushed a commit that referenced this issue Dec 18, 2024
#943 Fix flakey bart robot unit test
@olliesilvester
Copy link
Collaborator Author

This came up again

@olliesilvester olliesilvester reopened this Jan 7, 2025
@DominicOram
Copy link
Contributor

DominicOram commented Jan 21, 2025

I think we need to properly mock it and rethink how the test works rather than just change the length of sleeps. e.g. we could patch sleep like in

async def test_when_thawing_triggered_then_turn_on_sleep_and_turn_off(
so that we can be very specific on when things should be happening

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants