Skip to content
This repository was archived by the owner on Sep 2, 2024. It is now read-only.

973 speed up test pin centring#1023

Merged
DominicOram merged 5 commits into
mainfrom
973_speed_up_test_pin_centring
Dec 8, 2023
Merged

973 speed up test pin centring#1023
DominicOram merged 5 commits into
mainfrom
973_speed_up_test_pin_centring

Conversation

@5u6in

@5u6in 5u6in commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

Fixes #973

  • Identified test functions test_pin_tip_starting_near_negative_edge_doesnt_exceed_limit and test_pin_tip_starting_near_positive_edge_doesnt_exceed_limit that took the most time to run
  • Created mock object and mock generator to replace call to yield from trigger_and_return_pin_tip(oav.mxsc.pin_tip) in two test functions that took the most time
  • Added .side_effect to mock generators

To test:

  1. Run tests in test_pin_tip_centring.py in main branch and compare how long they take with 973_speed_up_test_pin_centring branch
  2. Confirm branch is faster using the Testing feature in VSCode

@5u6in
5u6in requested a review from DominicOram December 6, 2023 10:52
@5u6in 5u6in self-assigned this Dec 6, 2023
@olliesilvester
olliesilvester self-requested a review December 7, 2023 11:26
):
mock_move_pin_into_view.side_effect = [
mock_generator(0, 0),
mock_generator(0, 0),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should: The return value for mock_generator gives us our tip_x and tip_y, so we should give it the same parameters as it was using before, in the sim_puts, (0,100).

)


def mock_generator(x, y):

@olliesilvester olliesilvester Dec 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could: We are using this function to mock out the trigger_and_return_pin_tip, and it gives us a pin_x and pin_y value. I'd name it something like get_fake_pin_values_generator instead

@patch("hyperion.experiment_plans.pin_tip_centring_plan.trigger_and_return_pin_tip")
def test_pin_tip_starting_near_negative_edge_doesnt_exceed_limit(
smargon: Smargon, oav: OAV, RE: RunEngine
mock_move_pin_into_view: MagicMock, smargon: Smargon, oav: OAV, RE: RunEngine

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should: This variable is the patched mock for trigger_and_return_pin_tip, so mock_move_pin_into_view should be called mock_trigger_and_return_pin_tip instead

@patch("hyperion.experiment_plans.pin_tip_centring_plan.trigger_and_return_pin_tip")
def test_pin_tip_starting_near_positive_edge_doesnt_exceed_limit(
smargon: Smargon, oav: OAV, RE: RunEngine
mock_move_pin_into_view: MagicMock, smargon: Smargon, oav: OAV, RE: RunEngine

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same comment as above for this variable name

@olliesilvester olliesilvester left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, these tests run more than 2x faster than on main. There's just a few very minor changes I've put in the comments

@5u6in
5u6in marked this pull request as draft December 7, 2023 15:06
@5u6in
5u6in marked this pull request as ready for review December 7, 2023 15:06
@olliesilvester

Copy link
Copy Markdown
Contributor

Looks good now, thanks. I'm stumped on why the CI is failing though, any ideas @dperl-dls ?

@d-perl

d-perl commented Dec 7, 2023

Copy link
Copy Markdown
Contributor

@olliesilvester Just need to merge main if you want tests to pass, related to recent ispyb changes. I think it's fine to just merge this, CI will be fine in main

@codecov

codecov Bot commented Dec 8, 2023

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (369e8da) 93.68% compared to head (fd29564) 93.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1023      +/-   ##
==========================================
- Coverage   93.68%   93.50%   -0.19%     
==========================================
  Files          56       56              
  Lines        2708     2708              
==========================================
- Hits         2537     2532       -5     
- Misses        171      176       +5     

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

@DominicOram DominicOram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, I'm glad our tests are faster now. Thanks @subinsaji!

@DominicOram
DominicOram merged commit 5b901ba into main Dec 8, 2023
@DominicOram
DominicOram deleted the 973_speed_up_test_pin_centring branch December 8, 2023 10:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Speed up test_pin_tip_centring.py

4 participants