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: Refactored tests that were still using the tap mocha shim + chai to now use tap. #1919

Merged
merged 1 commit into from Dec 20, 2023

Conversation

bizob2828
Copy link
Member

Description

This PR refactored the remaining tests that still relied on mocha + chai. I also consolidated some of the custom test assertions into test/lib/custom-assertions.js. Lastly, I refactored the metrics helpers to not longer use assert and/or chai and use tap. With all of that I was able to remove chai from the devDependency list. This will also help developers triaging failulres in tests as they are now all tap assertions. When relying on mocha + chai it was very difficult at times, or even worse relying on the built in assert package.

@bizob2828 bizob2828 added the dev:tests Indicates only changes to tests label Dec 19, 2023
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2d666b6) 96.98% compared to head (da61b76) 96.97%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1919      +/-   ##
==========================================
- Coverage   96.98%   96.97%   -0.01%     
==========================================
  Files         212      212              
  Lines       40060    40036      -24     
==========================================
- Hits        38853    38826      -27     
- Misses       1207     1210       +3     
Flag Coverage Δ
integration-tests-16.x 78.70% <ø> (+0.01%) ⬆️
integration-tests-18.x 78.96% <ø> (+0.02%) ⬆️
integration-tests-20.x 78.98% <ø> (+0.02%) ⬆️
unit-tests-16.x 91.00% <ø> (-0.02%) ⬇️
unit-tests-18.x 90.98% <ø> (-0.02%) ⬇️
unit-tests-20.x 90.98% <ø> (-0.02%) ⬇️
versioned-tests-16.x 73.76% <ø> (-0.04%) ⬇️
versioned-tests-18.x 73.78% <ø> (-0.04%) ⬇️
versioned-tests-20.x 73.79% <ø> (-0.04%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jsumners-nr jsumners-nr left a comment

Choose a reason for hiding this comment

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

Fantastic work. I didn't read everything, but the pattern looked clear. Great job sticking to just the minimum necessary.

@@ -71,639 +65,572 @@ function FakeSegment(transaction, duration, name = 'FakeSegment') {
}
}

const helper = (module.exports = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like that this sort of declaration has been removed. It's quite difficult to read and keep track of. I think the refactored declaration is also going to help quite a lot with jsdoc and ide integration. The only thing missing from the refactor is the removal of the singleton _agent.

Copy link
Member Author

Choose a reason for hiding this comment

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

yea this was something I wasn't planning on doing but had to edit a few things here so broke this down. The agent singleton is going to be a heavier lift. I don't know the effects of this. I can log a story in our cleanup feature for this

// Min port: 1024 (without root)
// Max port: 65535
// Our range: 1024-65024
const port = Math.ceil(Math.random() * 64000 + 1024)
Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, this could be:

Suggested change
const port = Math.ceil(Math.random() * 64000 + 1024)
const port = require('crypto').randomInt(1024, 65024)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to stick with this kind of refactor in a diff PR, I agree though. I'll queue that one up later

@mrickard mrickard self-assigned this Dec 20, 2023
@bizob2828 bizob2828 merged commit 957529e into newrelic:main Dec 20, 2023
21 of 22 checks passed
Node.js Engineering Board automation moved this from Needs PR Review to Done: Issues recently completed Dec 20, 2023
@github-actions github-actions bot mentioned this pull request Jan 3, 2024
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

3 participants