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

[NoQA] Skip over a failing UnreadIndicatorsTest test #14363

Merged
merged 1 commit into from
Jan 17, 2023

Conversation

mountiny
Copy link
Contributor

@mountiny mountiny commented Jan 17, 2023

Details

A test is failing now because we have updated how local notifications are being triggered and this influences the test. Fixing it might take a bit, so we are skipping it so we can merge PRs just fine.

Fixed Issues

Partially #14352

Tests

N/A

  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

N/A

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Only touching tests, no need for screenshots.

Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android

@mountiny mountiny self-assigned this Jan 17, 2023
@mountiny mountiny marked this pull request as ready for review January 17, 2023 18:00
@mountiny mountiny requested a review from a team as a code owner January 17, 2023 18:00
@melvin-bot melvin-bot bot requested review from alex-mechler and removed request for a team January 17, 2023 18:01
@melvin-bot
Copy link

melvin-bot bot commented Jan 17, 2023

@alex-mechler Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@mountiny
Copy link
Contributor Author

mountiny commented Jan 17, 2023

Reviewer Checklist

Got approval here https://expensify.slack.com/archives/C01GTK53T8Q/p1673978615937299?thread_ts=1673916481.148719&cid=C01GTK53T8Q

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android

@mountiny mountiny merged commit 3636f1f into main Jan 17, 2023
@mountiny mountiny deleted the vit-skipOverUnreadIndicatorTest branch January 17, 2023 18:06
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 695.215 ms → 709.659 ms (+14.444 ms, +2.1%)
Open Search Page TTI 599.058 ms → 610.023 ms (+10.966 ms, +1.8%)
App start runJsBundle 190.531 ms → 194.581 ms (+4.049 ms, +2.1%)
App start nativeLaunch 20.844 ms → 20.969 ms (+0.125 ms, +0.6%)
App start regularAppStart 0.015 ms → 0.016 ms (+0.000 ms, +2.1%)
Show details
Name Duration
App start TTI Baseline
Mean: 695.215 ms
Stdev: 26.175 ms (3.8%)
Runs: 654.6712580001913 656.3709040000103 659.5229899999686 662.8895310000516 665.5882890000939 667.4913559998386 667.497955000028 668.6912110000849 670.5749349999242 673.4568699998781 677.7166539998725 683.5102789998055 686.9242739998735 691.2492639999837 692.9709910000674 696.012974999845 696.185202000197 696.9333239998668 699.4063430000097 700.248013000004 704.6856570001692 708.4185080002062 708.9625160000287 715.7855150001124 716.3410020000301 717.719713000115 719.6170910000801 723.6120879999362 735.9655240001157 737.4114749999717 744.7326899999753 745.7121890000999

Current
Mean: 709.659 ms
Stdev: 26.212 ms (3.7%)
Runs: 656.0537899998017 667.7313600000925 673.8225650000386 675.9834349998273 676.7664089999162 681.0582750001922 684.9680599998683 689.3083799998276 690.9985639997758 692.1045360001735 695.6004960001446 699.7844290002249 700.1574160000309 700.7318110000342 708.4696960002184 709.0919490000233 712.7079190001823 713.4050230002031 715.3368600001559 715.434431000147 725.0542210000567 725.1260350001976 725.2615939998068 730.0413080002181 730.4761399999261 731.354234999977 737.5644879997708 738.4965610001236 743.3926099999808 748.8518229997717 756.6441370001994 757.2951199999079
Open Search Page TTI Baseline
Mean: 599.058 ms
Stdev: 14.968 ms (2.5%)
Runs: 577.7896730001085 578.6870929999277 579.1198729998432 581.3139240001328 584.7764489999972 584.9149179998785 587.9298910000362 588.9985759998672 589.6205239999108 589.881876999978 591.8616539998911 592.7325439997949 594.2830409999005 594.5261229998432 594.8568110000342 596.2244470003061 599.4940189998597 601.4065350000747 602.1124279997312 602.4369719997048 602.6425779997371 602.7325839996338 604.6279299999587 607.3623460000381 608.3366300002672 609.7045090002939 613.0977779999375 615.6146649997681 616.222087000031 630.5141199999489 646.9619140001014

Current
Mean: 610.023 ms
Stdev: 21.377 ms (3.5%)
Runs: 571.5714920000173 576.9139809999615 580.0120450002141 588.4824230000377 589.3330490002409 591.4398610005155 591.7779140006751 592.7300619995221 592.8518479997292 593.6291910000145 594.638184000738 597.4088139999658 599.2013759994879 600.868815000169 602.1056730002165 604.5327559998259 605.8640130003914 610.423014999833 611.5520019996911 615.457723999396 617.1322839995846 619.121907999739 620.2546799993142 621.5556239997968 627.2297769999132 627.5132649997249 630.6469320002943 631.240275000222 631.9444989999756 641.5953369997442 644.6549889994785 645.8045250000432 661.2841400001198
App start runJsBundle Baseline
Mean: 190.531 ms
Stdev: 18.420 ms (9.7%)
Runs: 165 166 169 170 171 172 177 177 179 181 182 183 183 184 185 186 188 188 188 189 190 191 194 203 204 206 206 208 216 223 232 241

Current
Mean: 194.581 ms
Stdev: 14.045 ms (7.2%)
Runs: 165 168 172 175 176 178 186 186 187 190 191 192 193 194 194 196 197 198 199 199 200 201 201 204 205 207 211 214 216 218 219
App start nativeLaunch Baseline
Mean: 20.844 ms
Stdev: 2.938 ms (14.1%)
Runs: 17 17 17 18 18 18 18 18 18 18 18 19 20 20 20 20 21 21 21 21 21 22 23 23 24 24 24 24 25 25 27 27

Current
Mean: 20.969 ms
Stdev: 2.974 ms (14.2%)
Runs: 18 18 18 18 18 18 18 18 19 19 19 19 19 20 20 20 20 20 21 21 21 22 22 22 22 24 25 25 25 26 27 29
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (7.5%)
Runs: 0.013101999647915363 0.013671999797224998 0.013671999797224998 0.013996999710798264 0.014078999869525433 0.0143630001693964 0.014404000248759985 0.014485000167042017 0.014485000167042017 0.01485099969431758 0.015056000091135502 0.015136000234633684 0.015217999927699566 0.01525900000706315 0.01525900000706315 0.015298999845981598 0.015300000086426735 0.015339999925345182 0.015422000084072351 0.015462999697774649 0.015503000002354383 0.015544000081717968 0.01582799991592765 0.015828999690711498 0.015949999913573265 0.0166830001398921 0.01704900013282895 0.017090000212192535 0.017130999825894833 0.017253000289201736 0.018188000191003084

Current
Mean: 0.016 ms
Stdev: 0.001 ms (5.2%)
Runs: 0.014037999790161848 0.014607999939471483 0.01464799977838993 0.014689000323414803 0.0147299999371171 0.014891999773681164 0.015054999850690365 0.015054999850690365 0.015177999623119831 0.015258999541401863 0.01525900000706315 0.015339999925345182 0.015542999841272831 0.015542999841272831 0.015544000081717968 0.015625 0.015665999613702297 0.015746999997645617 0.0157880000770092 0.01582799991592765 0.01582799991592765 0.015868999995291233 0.015868999995291233 0.015950999688357115 0.01607200037688017 0.016682999674230814 0.01672299997881055 0.01713100029155612 0.017212000209838152 0.017212000209838152 0.017293000128120184

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @mountiny in version: 1.2.56-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.2.56-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants