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.

Sign up for GitHub

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

Jump to bottom

test: Fix flaky App Start Tests #4299

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

test: Fix flaky App Start Tests #4299

wants to merge 4 commits into from
+9 −0

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Nov 21, 2024 •
edited
Loading

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Mocks Date.now() call so that no delay is introduced during testing.

💡 Motivation and Context

Fixes #4197

Checking ten different runs that resulted in failure, the cause was the timestamp comparison tolerance. By default the test requires 2 decimal digits precision (Math.abs(expected - received) < 0.005) but that fails frequently on CI on the following tests:

  • Standalone App Start › adds ui kit init start mark as a child of the main app start span
  • Standalone App Start › adds bundle execution before react root via private api (used by Sentry.wrap())
  • Standalone App Start › Adds bundle execution span
Test Expected Actual Diff
Run 1729519015.909 1729519015.926 0.017
Run 1731675423.905 1731675423.927 0.022
Run 1732180783.445 1732180783.467 0.022
Run 1731580375.892 1731580375.93 0.038
Run 1731512750.846 1731512750.854 0.008
Run 1729218591.256 1729218591.263 0.007
Run 1730104950.111 1730104950.122 0.011
Run 1729068194.61 1729068194.618 0.008
Run 1729000213.109 1729000213.114 0.005
Run 1730130197.896 1730130197.909 0.013

To avoid any unexpected introduced delay the Date.now() is mocked.

💚 How did you test it?

CI (multiple executions of the failed check), Locally by increasing the expected timestamp comparison accuracy to verify that no delay is introduced.

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 21, 2024 •
edited
Loading

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 462.44 ms 450.32 ms -12.12 ms
Size 17.74 MiB 20.08 MiB 2.35 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
5446992 403.40 ms 426.70 ms 23.30 ms
700cbf4 425.56 ms 436.26 ms 10.70 ms
15c80ab+dirty 336.27 ms 350.58 ms 14.31 ms
8ae23a7 526.83 ms 513.38 ms -13.45 ms
12427f4 393.69 ms 414.84 ms 21.14 ms
e1ea4a8 506.82 ms 510.48 ms 3.66 ms
e5c9b8b 409.02 ms 426.66 ms 17.64 ms
5571a20 410.55 ms 441.06 ms 30.51 ms
fe13591 478.92 ms 480.84 ms 1.92 ms
9c48b2c 349.24 ms 385.96 ms 36.72 ms

App size

Revision Plain With Sentry Diff
5446992 17.73 MiB 19.85 MiB 2.12 MiB
700cbf4 17.73 MiB 20.07 MiB 2.33 MiB
15c80ab+dirty 17.73 MiB 20.04 MiB 2.31 MiB
8ae23a7 17.74 MiB 20.07 MiB 2.34 MiB
12427f4 17.73 MiB 19.85 MiB 2.12 MiB
e1ea4a8 17.74 MiB 20.08 MiB 2.34 MiB
e5c9b8b 17.73 MiB 19.83 MiB 2.10 MiB
5571a20 17.73 MiB 19.93 MiB 2.19 MiB
fe13591 17.74 MiB 20.07 MiB 2.34 MiB
9c48b2c 17.73 MiB 19.80 MiB 2.07 MiB

Previous results on branch: antonis/4197-appStartTesttolerance

Startup times

Revision Plain With Sentry Diff
d4b476a 679.40 ms 669.44 ms -9.96 ms

App size

Revision Plain With Sentry Diff
d4b476a 17.74 MiB 20.08 MiB 2.35 MiB

@antonis antonis marked this pull request as ready for review November 21, 2024 12:51
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 21, 2024 •
edited
Loading

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 381.37 ms 423.50 ms 42.13 ms
Size 7.15 MiB 8.35 MiB 1.21 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
7fd512a+dirty 439.69 ms 503.71 ms 64.01 ms
0d3e677+dirty 384.24 ms 431.45 ms 47.21 ms
15c80ab+dirty 276.38 ms 327.54 ms 51.17 ms
0db0c72+dirty 335.20 ms 351.06 ms 15.86 ms
e2b64fe+dirty 258.82 ms 304.26 ms 45.44 ms
1c65324+dirty 381.10 ms 427.26 ms 46.16 ms
5a22220+dirty 384.61 ms 419.06 ms 34.45 ms
728164b+dirty 335.93 ms 342.94 ms 7.01 ms
62a750b+dirty 370.78 ms 376.73 ms 5.96 ms
c398f67+dirty 315.08 ms 345.60 ms 30.52 ms

App size

Revision Plain With Sentry Diff
7fd512a+dirty 7.15 MiB 8.35 MiB 1.21 MiB
0d3e677+dirty 7.15 MiB 8.35 MiB 1.20 MiB
15c80ab+dirty 7.15 MiB 8.09 MiB 966.13 KiB
0db0c72+dirty 7.15 MiB 8.04 MiB 911.02 KiB
e2b64fe+dirty 7.15 MiB 8.07 MiB 947.16 KiB
1c65324+dirty 7.15 MiB 8.22 MiB 1.07 MiB
5a22220+dirty 7.15 MiB 8.21 MiB 1.06 MiB
728164b+dirty 7.15 MiB 8.12 MiB 997.71 KiB
62a750b+dirty 7.15 MiB 8.21 MiB 1.06 MiB
c398f67+dirty 7.15 MiB 8.21 MiB 1.07 MiB

Previous results on branch: antonis/4197-appStartTesttolerance

Startup times

Revision Plain With Sentry Diff
d4b476a+dirty 395.56 ms 440.46 ms 44.90 ms

App size

Revision Plain With Sentry Diff
d4b476a+dirty 7.15 MiB 8.35 MiB 1.21 MiB

@krystofwoldrich
Copy link
Member

Would it be possible to track down the value that's not correctly mocked?

I believe that's the core issue of the flakiness.

We mock the app start and current time timestamps, the time should not be changing.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 21, 2024 •
edited
Loading

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1235.31 ms 1234.08 ms -1.22 ms
Size 2.36 MiB 3.10 MiB 753.34 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
77680ec+dirty 1231.81 ms 1238.35 ms 6.54 ms
1faf8e3+dirty 1214.87 ms 1222.83 ms 7.97 ms
7bc4d75+dirty 1233.40 ms 1229.56 ms -3.83 ms
d7401ac+dirty 1252.38 ms 1275.04 ms 22.66 ms
8ae23a7+dirty 1230.02 ms 1227.62 ms -2.40 ms
5a22220+dirty 1209.49 ms 1220.94 ms 11.45 ms
80b2ce3+dirty 1265.92 ms 1268.60 ms 2.69 ms
b95b8af+dirty 1221.39 ms 1228.52 ms 7.13 ms
d0bf494+dirty 1289.40 ms 1298.40 ms 9.00 ms
f06c879+dirty 1252.64 ms 1259.66 ms 7.02 ms

App size

Revision Plain With Sentry Diff
77680ec+dirty 2.36 MiB 3.10 MiB 753.42 KiB
1faf8e3+dirty 2.36 MiB 3.08 MiB 736.75 KiB
7bc4d75+dirty 2.36 MiB 3.10 MiB 752.58 KiB
d7401ac+dirty 2.36 MiB 2.83 MiB 481.14 KiB
8ae23a7+dirty 2.36 MiB 3.10 MiB 752.42 KiB
5a22220+dirty 2.36 MiB 2.92 MiB 570.21 KiB
80b2ce3+dirty 2.36 MiB 2.84 MiB 486.98 KiB
b95b8af+dirty 2.36 MiB 3.14 MiB 793.32 KiB
d0bf494+dirty 2.36 MiB 2.83 MiB 481.15 KiB
f06c879+dirty 2.36 MiB 2.88 MiB 530.42 KiB

Previous results on branch: antonis/4197-appStartTesttolerance

Startup times

Revision Plain With Sentry Diff
d4b476a+dirty 1221.92 ms 1226.78 ms 4.86 ms

App size

Revision Plain With Sentry Diff
d4b476a+dirty 2.36 MiB 3.10 MiB 753.33 KiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 21, 2024 •
edited
Loading

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1244.51 ms 1241.78 ms -2.73 ms
Size 2.92 MiB 3.66 MiB 758.61 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
77680ec+dirty 1237.65 ms 1236.24 ms -1.41 ms
1faf8e3+dirty 1223.38 ms 1220.56 ms -2.82 ms
7bc4d75+dirty 1222.13 ms 1216.39 ms -5.74 ms
d7401ac+dirty 1288.10 ms 1289.54 ms 1.44 ms
8ae23a7+dirty 1233.67 ms 1229.52 ms -4.15 ms
5a22220+dirty 1246.18 ms 1249.61 ms 3.43 ms
80b2ce3+dirty 1245.12 ms 1262.04 ms 16.92 ms
b95b8af+dirty 1235.60 ms 1242.06 ms 6.46 ms
d0bf494+dirty 1266.20 ms 1267.52 ms 1.32 ms
f06c879+dirty 1285.14 ms 1285.86 ms 0.72 ms

App size

Revision Plain With Sentry Diff
77680ec+dirty 2.92 MiB 3.66 MiB 758.54 KiB
1faf8e3+dirty 2.92 MiB 3.64 MiB 742.61 KiB
7bc4d75+dirty 2.92 MiB 3.66 MiB 757.15 KiB
d7401ac+dirty 2.92 MiB 3.40 MiB 488.06 KiB
8ae23a7+dirty 2.92 MiB 3.66 MiB 757.67 KiB
5a22220+dirty 2.92 MiB 3.48 MiB 575.81 KiB
80b2ce3+dirty 2.92 MiB 3.40 MiB 492.75 KiB
b95b8af+dirty 2.92 MiB 3.69 MiB 794.16 KiB
d0bf494+dirty 2.92 MiB 3.40 MiB 488.08 KiB
f06c879+dirty 2.92 MiB 3.44 MiB 533.24 KiB

Previous results on branch: antonis/4197-appStartTesttolerance

Startup times

Revision Plain With Sentry Diff
d4b476a+dirty 1229.50 ms 1232.55 ms 3.05 ms

App size

Revision Plain With Sentry Diff
d4b476a+dirty 2.92 MiB 3.66 MiB 758.60 KiB

@antonis antonis changed the title test: Increase appStart timestamp comparison tolerance test: Fix flaky App Start Tests Nov 21, 2024
@antonis
Copy link
Collaborator Author

antonis commented Nov 21, 2024

Would it be possible to track down the value that's not correctly mocked?

I believe that's the core issue of the flakiness.

We mock the app start and current time timestamps, the time should not be changing.

Thank you for the feedback @krystofwoldrich 🙇

The only thing that I found un-mocked is the date.now call that may justify the random delay causing the tests to fail. I updated the code and the PR description.
Any further feedback is more than welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich krystofwoldrich is a code owner

@lucas-zimerman lucas-zimerman Awaiting requested review from lucas-zimerman lucas-zimerman is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Flaky App Start Test
2 participants
@antonis @krystofwoldrich