-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[NoQA] Increase number of e2e test runs #28822
[NoQA] Increase number of e2e test runs #28822
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I'm missing something but wouldn't this reduce accuracy?
I mistakenly thought this was reducing from 90 > 30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don think this should reduce accuracy, you will have more attempts so the average will be more accurate in theory.
However, does this mean the tests will run 3 times longer. It already runs quite a bit and we dont have that many tests
@Julesssss systems stabilize over time, it's like flipping a coin more times, over time the real tendency (e.g. 50-50) emerges from the noise. |
Ignore me, I thought we were REDUCING from 90 to 30 😅 |
Reviewer Checklist
Screenshots/VideosWebN/A - Just tests Mobile Web - ChromeN/A - Just tests Mobile Web - SafariN/A - Just tests DesktopN/A - Just tests iOSN/A - Just tests AndroidN/A - Just tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think test time should go up by any significant amount. 95% of the time related to the test is building the APK and getting AWS spun up.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.3.78-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.78-4 🚀
|
@Julesssss no no, we are going from 30 → 90 @AndrewGable After further testing on my local environment I can definitely see a performance regression with the tests running on a single emulator instance. I can imagine the same is happening on the cloud instance of the device. While increasing the runs will get rid of statistical noise there is clearly an issue where running multiple tests on a single instance degrades performance of the device over time. |
🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.3.79-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
Details
It appears e2e tests are miss-detecting performance regressions. Will try to debug the issues. First idea is to increase the numbers of iterations to try to reduce statistical noice.
Fixed Issues
$ #28735 (comment)
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android