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: updated the grpc versioned tests utils to dynamically bind ports to avoid conflicts between cjs and esm tests #1820

Merged
merged 1 commit into from Oct 17, 2023

Conversation

bizob2828
Copy link
Member

We started seeing failing versioned tests when running in CI after larger runners. It was always grpc tests with:

# Subtest: should record errors in a transaction when ignoring 
        not ok 1 - No address added out of total 1 resolved
          ---
          stack: >
            bindResultPromise.then.errorString
            (node_modules/@grpc/grpc-js/src/server.ts:569:32)
          at:
            line: 569
            column: 32
            file: node_modules/@grpc/grpc-js/src/server.ts
            function: bindResultPromise.then.errorString
          test: "should record errors in a transaction when ignoring "
          source: |2
                          logging.log(LogVerbosity.ERROR, errorString);
                          deferredCallback(new Error(errorString), 0);
            -------------------------------^
                        } else {
                          if (bindResult.count < addressList.length) {

After looking at the tests I realized we bind to a static port. We never hit this before because the grpc and grpc-esm tests were never run at the same time due to concurrency.

To verify this works locally:

node_modules/.bin/versioned-tests --major -i 2 --all --strict --samples 10 test/versioned/grpc test/versioned/grpc-esm/

Previously, this would immediately fail on one of the 2 test suites because it was trying to bind to the same port.

…s to avoid conflicts between cjs and esm tests
@bizob2828 bizob2828 added the dev:tests Indicates only changes to tests label Oct 16, 2023
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #1820 (b762b26) into main (99e5792) will not change coverage.
Report is 7 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1820   +/-   ##
=======================================
  Coverage   96.83%   96.83%           
=======================================
  Files         199      199           
  Lines       38959    38959           
=======================================
  Hits        37724    37724           
  Misses       1235     1235           
Flag Coverage Δ
integration-tests-16.x 78.91% <ø> (ø)
integration-tests-18.x 79.19% <ø> (ø)
integration-tests-20.x 79.19% <ø> (ø)
unit-tests-16.x 91.43% <ø> (ø)
unit-tests-18.x 91.41% <ø> (ø)
unit-tests-20.x 91.41% <ø> (ø)
versioned-tests-16.x 73.05% <ø> (-0.01%) ⬇️
versioned-tests-18.x 73.05% <ø> (-0.01%) ⬇️
versioned-tests-20.x 73.06% <ø> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mrickard mrickard self-assigned this Oct 17, 2023
@bizob2828 bizob2828 merged commit 95ac917 into newrelic:main Oct 17, 2023
29 checks passed
Node.js Engineering Board automation moved this from Needs PR Review to Done: Issues recently completed Oct 17, 2023
This was referenced Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev:tests Indicates only changes to tests
Projects
Node.js Engineering Board
  
Done: Issues recently completed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants