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

test(fetch): make prepareData async to enforce fakeTime order #2969

Merged
merged 7 commits into from
May 30, 2022

Conversation

Ugzuzg
Copy link
Contributor

@Ugzuzg Ugzuzg commented May 13, 2022

Which problem is this PR solving?

Trying to resolve flaky fetch instrumentation tests.

Fixes #2852

Short description of the changes

I was able to reproduce the issue by supplying half a cpu to a docker container in which the tests are running.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Repeated the test execution multiple times in a container with half a cpu.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented May 13, 2022

Codecov Report

Merging #2969 (d4e3750) into main (e44f6d1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2969   +/-   ##
=======================================
  Coverage   92.77%   92.77%           
=======================================
  Files         183      183           
  Lines        6134     6134           
  Branches     1300     1300           
=======================================
  Hits         5691     5691           
  Misses        443      443           

@Ugzuzg Ugzuzg marked this pull request as ready for review May 13, 2022 15:33
@Ugzuzg Ugzuzg requested a review from a team May 13, 2022 15:33
@dyladan
Copy link
Member

dyladan commented May 13, 2022

Looks like coverage changes imply the browser tests no longer trigger the isBrowser check in the browser detector. I'm not sure what in the diff would have caused that

@Ugzuzg Ugzuzg force-pushed the test/instrumentation-fetch branch from 0484412 to b1d9b86 Compare May 14, 2022 10:42
@Ugzuzg
Copy link
Contributor Author

Ugzuzg commented May 14, 2022

@dyladan, seems to be an issue with coverage reporting. The reports are uploaded under different commit hashes for some reason. Both runs belong to Unit Tests #4251:

Looks better after rerunning, but it is indeed strange. The same thing happened in my other PR, I think.

@Ugzuzg Ugzuzg force-pushed the test/instrumentation-fetch branch from b1d9b86 to b8604d9 Compare May 19, 2022 14:47
@rauno56
Copy link
Member

rauno56 commented May 25, 2022

Any pointers on how to repro, @Ugzuzg ?

@Ugzuzg
Copy link
Contributor Author

Ugzuzg commented May 25, 2022

@rauno56, I'm mounting this repo in a container with half a cpu: docker run --cpu 0.5 -v $(pwd):/app -it circleci/node:16-browsers /bin/bash. Then building and running the tests of fetch instrumentation. They're failing on almost every execution on main.

@Ugzuzg Ugzuzg force-pushed the test/instrumentation-fetch branch from b8604d9 to 2b2b70e Compare May 25, 2022 19:56
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

LGTM thanks for looking into this

@dyladan
Copy link
Member

dyladan commented May 27, 2022

@rauno56 the race you mentioned is removed. Would appreciate a ✅ and we can merge

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

I really appreciate the effort you've put into it finding out the root cause, @Ugzuzg. Thanks!

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for putting in the time and effort to work on this. 🙂

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Thank you for your work on this!

…/fetch.test.ts

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
@legendecas legendecas merged commit fd95d42 into open-telemetry:main May 30, 2022
@legendecas
Copy link
Member

Landed. Thank you for your contribution!

@Ugzuzg Ugzuzg deleted the test/instrumentation-fetch branch May 30, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky browser test
6 participants