-
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] fix: e2e typing test #48191
[NoQA] fix: e2e typing test #48191
Conversation
@kirillzyusko Does this need a C+ review since it's NoQA? |
This is for internal dev tooling, no C+ needed 😊 |
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.
Approving blind to get an engineer assigned 🙏
Bringing in @mountiny here since I'm not sure I'd be a good reviewer for this... |
@mountiny would you mind to have a look on that? 👀 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
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 think we can move ahead with this one, the changes look good, thanks for investigating this @kirillzyusko hopefully this will be fixed for some time now
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.28-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.28-3 🚀
|
Details
Fixed e2e typing test.
The failure was very strange - I haven't seen any warnings/errors in adb logcat. The app just hangs, but if we touch the screen, then Android will show ANR and will close the app 🤷♂️
I have tried to debug the code but discovered that the problem could be fixed if we request focus later. I tried 1ms, 16ms, 32ms but all these values still lead app to frozen state. 1000ms seems to work more reliable, but sometimes occasionally freezes the app (but 2000ms work fine).
My second thought was the assumption, that we need to request focus only after view being laid out - I've tried to request a focus from
onLayout
but it doesn't help (but it still better, because in StrictMode effects are running two times, and we have a race condition when request focus from effect, so I decided to keep this code).I also tried to update reanimated in a hope that it can fix the problem (in dev mode I got crashes coming from reanimated):
But with rea 3.15.0 I was still getting crashes.
With that fix e2e tests can actually pass:
I think it's not a perfect fix, but at least it'll fix pipeline - and in a meantime we can come up with a proper fix 💪
Fixed Issues
$ #48187
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop