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

ci: [Bug] DNS Lookup and Promise Lookup test for error NOT_FOUND failing #1136 #1138

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

MSNev
Copy link
Contributor

@MSNev MSNev commented Aug 30, 2022

Which problem is this PR solving?

[Bug] DNS Lookup and Promise Lookup test for error NOT_FOUND failing #1136

Short description of the changes

DNS resolution in local environment is exceeding 2 seconds (due to lots of fallback DNS servers) for invalid URL this is causing these 2 tests to timeout with the default 2 second delay.

Resolution:
Extend the timeout for these 2 tests so for environments where DNS resolution can take longer than expected the tests still pass.

@MSNev MSNev requested a review from a team August 30, 2022 18:19
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.

Seems fine as a short term solution but wouldn't it be better to mock the DNS and not rely on a slow external service?

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #1138 (e266d2b) into main (ec9ee13) will decrease coverage by 1.23%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1138      +/-   ##
==========================================
- Coverage   96.07%   94.84%   -1.24%     
==========================================
  Files          14       18       +4     
  Lines         892     1067     +175     
  Branches      191      217      +26     
==========================================
+ Hits          857     1012     +155     
- Misses         35       55      +20     
Impacted Files Coverage Δ
...tapackages/auto-instrumentations-node/src/utils.ts 98.00% <0.00%> (ø)
...lemetry-instrumentation-dns/src/instrumentation.ts 76.38% <0.00%> (ø)
...ry-instrumentation-dns/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...ode/opentelemetry-instrumentation-dns/src/utils.ts 95.91% <0.00%> (ø)

@MSNev
Copy link
Contributor Author

MSNev commented Aug 30, 2022

Seems fine as a short term solution but wouldn't it be better to mock the DNS and not rely on a slow external service?

I suspect most people won't hit this issue so I think mocking could be overkill.

@blumamir blumamir merged commit ff96b70 into open-telemetry:main Sep 1, 2022
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.

5 participants