-
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
[No QA] chore: add metrics for SearchPage #34489
[No QA] chore: add metrics for SearchPage #34489
Conversation
FYI Some linter errors |
@TMisiukiewicz @roryabraham Does this need C+ review? |
@mollfpr No i dont think so |
Can confirm, no C+ review 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.
LGTM but I think some basic test steps and screenshots are in order here.
ee936d1
to
e25f753
Compare
@roryabraham added tests steps and screenshots. Additionally, I also added tracking those metrics with |
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.
@TMisiukiewicz @adhorodyski one comment
Timing.end(CONST.TIMING.OPEN_SEARCH); | ||
Performance.markEnd(CONST.TIMING.OPEN_SEARCH); |
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.
If the timer never started given this component is used in other pages too, is that not going to cause any issues?
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.
@adhorodyski to confirm, no problem with this right?
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.
calling end without a start should be safe, I think. It will just return early
@mountiny thanks!! Rebasing this rn to keep it up to date. |
a1264c3
to
ff3da84
Compare
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: mWeb SafariMacOS: Desktop |
@TMisiukiewicz @adhorodyski minor piece of feedback – in E/App please don't rebase and force-push because it makes it harder to see the changes made since our last review. Just merge main into your branch and do a normal push instead. |
LGTM, and I think in this case testing on iOS and web (one metro platform, plus web) is probably good enough. The main reason I'm not merging is because I want to know if the reassure tests are a legitimate regression causing additional renders opening the search screen. It seems unlikely since the only effects added shouldn't cause any additional renders, but I'm not sure. |
No this was a flaky tests caused by some unnecessary providers in the mocked component. Once merged with main this hsould be resolved. |
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.
Noting from Slack, the failures are known and unrelated, going to merge
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
See above comment |
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.32-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.32-5 🚀
|
Details
Added metrics to track the performance of opening the search page, starting from pressing the search icon and ending when list on the
SearchPage
is rendered. Additionally added a metric to trackgetOptions
execution time in this particular screen.Fixed Issues
$ #34323
PROPOSAL: n/a
Tests
load_search_options
andopen_search
are visible in the consoleOffline tests
QA Steps
load_search_options
andopen_search
are visible in the consolePR 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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label 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