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

chore: Converted agent unit tests to node:test #2411

Closed
wants to merge 2 commits into from

Conversation

jsumners-nr
Copy link
Contributor

@jsumners-nr jsumners-nr commented Jul 31, 2024

This PR resolved #2380.

There are couple of main differences between tap and node:test patterns:

  1. In node:test every test() is actually a promise.
  2. In node:test every test function is wrapped in a promise.
  3. In node:test there is not a subtest .end or .autoEnd concept.

So if we have a tap test like:

tap.test('some stuff', t => {
	t.test('one', t => {
		t.equal(1, 1)
	})

	t.test('two', t => {
		foo('bar', (error, val) => {
			t.error(error, 'no error should occur')
			t.equal(val, 'bar')
			t.end()
		})

		function foo(param, cb) {
			cb(param)
		}
	})
})

The equivalent in node:test would be:

test('some stuff', async t => {
	await t.test('one', t => {
		assert.equal(1, 1)
	})

	await t.test('two', async t => {
		const { promise, resolve } = Promise.withResolvers()

		foo('bar', (error, val) => {
			assert.equal(error, undefined, 'no error should occur')
			assert.equal(val, 'bar')
			resolve()
		})
		await promise

		function foo(param, cb) {
			cb(param)
		}
	})
})

@jsumners-nr jsumners-nr added the dev:tests Indicates only changes to tests label Jul 31, 2024
@jsumners-nr jsumners-nr force-pushed the issue-2380 branch 2 times, most recently from 5598a76 to 5e7ac3e Compare July 31, 2024 13:35
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was devised to replace the usage of nock in test/unit/agent/agent.test.js. I encountered an issue where one of the subtests was unable to be resolved due to a timing issue between promise resolution and the event loop finishing. Utilizing "real" network requests helped me resolve the problem. If I remember correctly, it was this test causing grief:

t.test('should not blow up when harvest cycle runs', (t) => {
agent.start(() => {
setTimeout(() => {
t.ok(redirect.isDone())
t.ok(connect.isDone())
t.ok(settings.isDone())
t.end()
}, 15)
})
})

Maybe the test could have been reworked with nock still being used, but I am less convinced of nock's utility nowadays than I used to be. While the pattern is not used by any tests in this PR, by using the approach in this PR we can do things like:

test('something', async t => {
	const { collector } = t.ctx
	const { promise, resolve = Promise.withResolvers()

	collector.addHandler('/some/endpoint', (req, res) => {
		// Verify that the client generated the correct request structure:
		assert.equal(req.url, '/some/endpoint?with=query_params')
		res.writeHead(200)
		res.end()
		resolve()
	})

	await promise
})

Yes, it is possible with nock, but, despite using it for many years, I find myself having to go reference the documentation every time I want to do something like that.

const helper = require('./agent_helper')
const fakeCert = require('./fake-cert')

class Collector {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be generic enough to utilize in all tests that need to talk to the remote collector. I haven't actually looked, but I bet we can replace the fake-collector references in test/integration/collector-remote-method.tap.js with this.

sinon.stub(agent.harvester, 'start')
t.after(() => sinon.restore())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sinon.restore() is the sole reason for updating to sinon@5 (and evidently sinon@18 is the current version). Basically, in sinon@5 the default import is itself a "sandbox" and thus obviates the need to const sandbox = sinon.sandbox() and do a dance around that for this simple test.

@jsumners-nr jsumners-nr marked this pull request as ready for review July 31, 2024 14:20
@jsumners-nr jsumners-nr requested a review from bizob2828 July 31, 2024 14:23
@bizob2828 bizob2828 deleted the branch newrelic:next July 31, 2024 17:00
@bizob2828 bizob2828 closed this Jul 31, 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
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants