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

Add instance=True to mock.create_autospec for speed improvement #38567

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

jhaigh0
Copy link
Contributor

@jhaigh0 jhaigh0 commented Jan 7, 2025

Description of work

See mantidproject/mantidimaging#2434 for description on why this is effective.

I've compared all tests changed in this pr before and after the change and received a performance improvement in all of them.
It is particularly effective if create_autospec is being called in a setUp method. Tests speed up somewhere between 50% and 20% most of the time. None of the tests were massive, so overall impact won't be large.

Fixes #38560

To test:

Just unit tests and code review. If you want to search for instances of create_autospec which I've missed, be my guest, but I think I've been thorough.

This does not require release notes because no effect on users


Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

Code Review

  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
  • Do the release notes conform to the release notes guide?

Functional Tests

  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

@jhaigh0 jhaigh0 added Maintenance Unassigned issues to be addressed in the next maintenance period. ISIS Team: Core Issue and pull requests managed by the Core subteam at ISIS Investigation A task to investigate options for future work labels Jan 7, 2025
@jhaigh0 jhaigh0 added this to the Release 6.12 milestone Jan 7, 2025
@jclarkeSTFC jclarkeSTFC self-assigned this Jan 13, 2025
Copy link
Contributor

@jclarkeSTFC jclarkeSTFC left a comment

Choose a reason for hiding this comment

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

Looks good, just found a typo.

Co-authored-by: James Clarke <139879523+jclarkeSTFC@users.noreply.github.com>
@jclarkeSTFC jclarkeSTFC changed the title Add instace=True to mock.create_autospec for speed improvement Add instance=True to mock.create_autospec for speed improvement Jan 13, 2025
@SilkeSchomann SilkeSchomann self-assigned this Jan 13, 2025
@SilkeSchomann SilkeSchomann merged commit c7f74cc into main Jan 13, 2025
10 checks passed
@SilkeSchomann SilkeSchomann deleted the 38560_instance_true_for_create_autospec branch January 13, 2025 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Investigation A task to investigate options for future work ISIS Team: Core Issue and pull requests managed by the Core subteam at ISIS Maintenance Unassigned issues to be addressed in the next maintenance period.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate use of instance=True for mock.create_autospec speed improvement
3 participants