perf: Speed up registry integration tests#8679
perf: Speed up registry integration tests#8679dsarlis wants to merge 4 commits intodfinity:masterfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
Dear @dsarlis, In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA. If you decide to agree with it, please visit this issue and read the instructions there. Once you have signed it, re-trigger the workflow on this PR to see if your code can be merged. — The DFINITY Foundation |
|
I cannot dismiss the review of github-bot as I do not have write access anymore. I would therefore request a member of the Governance team to do it on my behalf, the changes are only affecting tests, so they should be fine to merge. |
|
I also attach the results I got from a run in my local environment. Before: After Note that in both runs 1 test failed (but it's different), I assume this is some flakiness especially since the first run is on clean master (runtimes are prohibitively slow for me to run multiple times to get averages). |
basvandijk
added
the
security-review-passed
|
Hey @dsarlis! Nice to see you driving by! I see our Auto fix job is failing. I'll look into that tomorrow. For the moment could you run a |
|
Indeed, great to see you back, @dsarlis ! |
github-actions
bot
dismissed
jasonz-dfinity’s stale review
Review dismissed by automation script.
Thanks Bas, I fixed the formatting manually. |
|
@dsarlis Do you think that's flakiness? I'll restart the test to check ... |
@basvandijk No, I see now that this test explicitly does not want to run on a |
@basvandijk I pushed a commit to revert the test. |
4 participants
This is a drive-by improvement that I noticed. Simply replacing
state_machine_test_on_nns_subnetinstead oflocal_test_on_nns_subnetin registry integration tests results in significant speed up of the respective tests. Reductions in test runtimes range from about ~30% to ~60% (for the tests that were using this function).Note that there was one test where
local_test_on_nns_subnet_with_mutationsis used which does not have a state machine equivalent. As I was unsure about the amount of work involved, I left it outside this PR.