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

Update nock to v13 #2249

Closed
wants to merge 1 commit into from
Closed

Conversation

cole-easton
Copy link
Contributor

Which problem is this PR solving?

Update Nock to v13
Fixes #1303

Short description of the changes

Nock version was updated and the deprecated and unstable http.ClientRequest.abort() method was replaced with destroy(), to ensure the behavior of the tests was correct and stable.

@obecny
Copy link
Member

obecny commented Jun 2, 2021

what about replacing socketDelay in favour of delayConnection ?

@cole-easton
Copy link
Contributor Author

@obecny I only saw two occurrences of socketDelay, which I've replaced. If I am missing any, could you point them out? Nothing else shows up in the project search feature of my editor.

@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #2249 (8eed30d) into main (fd2410c) will decrease coverage by 0.46%.
The diff coverage is n/a.

❗ Current head 8eed30d differs from pull request most recent head 431c756. Consider uploading reports for the commit 431c756 to get more accurate results

@@            Coverage Diff             @@
##             main    #2249      +/-   ##
==========================================
- Coverage   92.80%   92.34%   -0.47%     
==========================================
  Files         145      128      -17     
  Lines        5226     4244     -982     
  Branches     1071      867     -204     
==========================================
- Hits         4850     3919     -931     
+ Misses        376      325      -51     
Impacted Files Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...ges/opentelemetry-instrumentation-http/src/http.ts 94.88% <0.00%> (-0.10%) ⬇️
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
...ation-xml-http-request/src/enums/AttributeNames.ts
...kages/opentelemetry-web/src/StackContextManager.ts
packages/opentelemetry-web/src/types.ts
...lemetry-exporter-collector/src/transformMetrics.ts
...es/opentelemetry-context-zone-peer-dep/src/util.ts
...ry-context-zone-peer-dep/src/ZoneContextManager.ts
packages/opentelemetry-web/src/utils.ts
... and 9 more

@obecny
Copy link
Member

obecny commented Jun 3, 2021

@obecny I only saw two occurrences of socketDelay, which I've replaced. If I am missing any, could you point them out? Nothing else shows up in the project search feature of my editor.

hmm I somehow missed those changes, thought there were more :), thx

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@obecny obecny added the enhancement New feature or request label Jun 3, 2021
@dyladan
Copy link
Member

dyladan commented Jun 30, 2021

This is mergeable but I don't have permission to update the branch

@vmarchaud
Copy link
Member

@cole-easton Could you please rebase the branch so we can merge it ? (If possible allow us maintainers to edit your branch so we can it ourselves before merging it)

@dyladan
Copy link
Member

dyladan commented Jul 21, 2021

I still cannot merge this without permission to update your branch

@cole-easton cole-easton requested a review from MSNev as a code owner July 22, 2021 01:27
@cole-easton
Copy link
Contributor Author

@vmarchaud @dyladan Sorry for the delay. I just rebased -- you should be able to merge now.

@dyladan
Copy link
Member

dyladan commented Jul 23, 2021

Rerunning tests to see if the browser test failure was a fluke. If not, you will need to fix the test.

@dyladan
Copy link
Member

dyladan commented Jul 27, 2021

Have you looked into the browser test failures?

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.

Hundreds of files changed and many things that seem to have already been done? Not sure what's going on but the tests are failing and there is something going on here other than just upgrading nock.

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm if tests are green

@vmarchaud vmarchaud requested a review from dyladan August 7, 2021 13:54
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 after merge conflicts fix

@dyladan
Copy link
Member

dyladan commented Nov 22, 2021

@cole-easton can you fix the conflicts so this can be merged?

@dyladan
Copy link
Member

dyladan commented Nov 30, 2021

@cole-easton bump

@dyladan dyladan mentioned this pull request Dec 6, 2021
@alolita
Copy link
Member

alolita commented Dec 7, 2021

Hi @dyladan - can you please re-assign this issue to @rltoSD so that these conflicts can be resolved and the PR can be completed. Thanks!

@vmarchaud
Copy link
Member

Hi @dyladan - can you please re-assign this issue to @rltoSD so that these conflicts can be resolved and the PR can be completed. Thanks!

I believe he need to comment on the issue/PR so we can assign it

@dyladan
Copy link
Member

dyladan commented Dec 7, 2021

@alolita i already did that at #2652

@dyladan
Copy link
Member

dyladan commented Dec 7, 2021

Closed in favor of #2652

@dyladan dyladan closed this Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update nock to v13 and use "delayConnection" instead of socketDelay
7 participants