Skip to content

Fix Ui tests receiving empty requests#3094

Merged
stefanosiano merged 3 commits intomainfrom
fix/ui-tests-empty-requests
Dec 27, 2023
Merged

Fix Ui tests receiving empty requests#3094
stefanosiano merged 3 commits intomainfrom
fix/ui-tests-empty-requests

Conversation

@stefanosiano
Copy link
Contributor

@stefanosiano stefanosiano commented Dec 12, 2023 •
edited
Loading

📜 Description

added a check to avoid processing requests with 0 bytes bodies in ui tests (probably useless)
added a check to drop duplicated envelopes

#skip-changelog

💡 Motivation and Context

Some envelopes were sent twice, corrupting the counter in MockRelay and making tests fail

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Dec 12, 2023 •
edited
Loading

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 401.70 ms 478.92 ms 77.21 ms
Size 1.72 MiB 2.27 MiB 558.42 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8838e01 387.41 ms 467.00 ms 79.59 ms
4e260b3 384.08 ms 477.56 ms 93.48 ms
93a76ca 377.41 ms 448.22 ms 70.81 ms
0bd723b 375.20 ms 452.41 ms 77.20 ms
86f0096 368.63 ms 446.92 ms 78.29 ms
0404ea3 332.47 ms 401.12 ms 68.66 ms
b172d4e 412.60 ms 492.68 ms 80.08 ms
93a76ca 397.30 ms 455.16 ms 57.87 ms
8ff8fd0 432.77 ms 495.18 ms 62.41 ms
7eccfdb 389.94 ms 461.29 ms 71.35 ms

App size

Revision Plain With Sentry Diff
8838e01 1.72 MiB 2.29 MiB 578.15 KiB
4e260b3 1.72 MiB 2.27 MiB 554.95 KiB
93a76ca 1.72 MiB 2.29 MiB 576.75 KiB
0bd723b 1.72 MiB 2.29 MiB 578.09 KiB
86f0096 1.72 MiB 2.29 MiB 576.50 KiB
0404ea3 1.72 MiB 2.29 MiB 577.52 KiB
b172d4e 1.72 MiB 2.29 MiB 578.09 KiB
93a76ca 1.72 MiB 2.29 MiB 576.75 KiB
8ff8fd0 1.72 MiB 2.27 MiB 558.15 KiB
7eccfdb 1.72 MiB 2.27 MiB 556.93 KiB

Previous results on branch: fix/ui-tests-empty-requests

Startup times

Revision Plain With Sentry Diff
c6297e8 416.56 ms 488.48 ms 71.92 ms
d151fc7 396.16 ms 476.76 ms 80.60 ms

App size

Revision Plain With Sentry Diff
c6297e8 1.72 MiB 2.27 MiB 554.95 KiB
d151fc7 1.72 MiB 2.27 MiB 558.42 KiB

@romtsn
Copy link
Member

romtsn commented Dec 18, 2023

@stefanosiano it looks like the requests are actually not empty-bodied, but they look super weird, cause they are not errors or transactions really https://github.com/getsentry/sentry-java/actions/runs/7219916222/job/19671823552#step:8:411

@stefanosiano
cleaned up MockRelay, removing requests that are not envelopes (we do…
fd2a7f1 
…n't have such)

added a check to discard already received envelopes, sent twice for any reason)
@stefanosiano
Update BaseUiTest.kt
fdce60e
@stefanosiano
Copy link
Contributor Author

stefanosiano commented Dec 22, 2023

@stefanosiano it looks like the requests are actually not empty-bodied, but they look super weird, cause they are not errors or transactions really https://github.com/getsentry/sentry-java/actions/runs/7219916222/job/19671823552#step:8:411

That should be the captured message event, just truncated in that log
The problem was that a test could send the same envelope twice, corrupting the IdlingResource counter, but it's not logged as failing test (maybe due to timing?)
So subsequent tests fail because the counter gets always corrupted, since it's not reset.
I think on Saucelabs it happened very rarely because we have a TestOrchestrator that kills the process and cleans everything on every test, while in the AGP matrix it seems it's not run, and our cleanup methods are not enough

@stefanosiano stefanosiano marked this pull request as ready for review December 22, 2023 14:22
romtsn
romtsn approved these changes Dec 27, 2023
View reviewed changes
Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@stefanosiano stefanosiano merged commit 937879e into main Dec 27, 2023
@stefanosiano stefanosiano deleted the fix/ui-tests-empty-requests branch December 27, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romtsn romtsn romtsn approved these changes

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

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@stefanosiano @romtsn