test(node): Add Node SDK integration test structure.

[onurtemizkan]

This PR introduces a structure for integration testing Node SDK,

Summary:

• Pluggable init, subject and server scripts are put together, to start a test scenario. My first attempt was to bundle them with Webpack and run on a separate thread, but it's not compatible with how nock works (requires to patch and listen on the same thread).
• The request bodies are matched against jest snapshots. Random or platform-specific properties are patched with expect.any(...). I'm still not sure about this approach as it can be too fragile with even small updates on SDK (especially stack-trace snapshots).
• cjs/esm builds are separately tested, but we transform esm builds to cjs beforehand with babel, as jest still doesn't support esm modules natively.
• Having a configurable server script per test will probably make it easier to run and test with databases, but I haven't tried that yet. It would be great to collect some ideas about that as well (My first idea was to have a set of Docker nodes with predefined ports, that we start running before all tests).

This implementation is very basic and there might be many improvements required when we actually start writing tests. It would be great if I can get feedback and reviews about the approach taken here, so we can build the rest on top of that.

cc: @AbhiPrasad, @lobsterkatie

Fixes: #4260

[github-actions]

size-limit report

Path	Base Size (a929462)	Current Size	Change
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.49 KB	19.49 KB	0%
@sentry/browser - ES5 CDN Bundle (minified)	62.17 KB	62.17 KB	0%
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.12 KB	18.11 KB	-0.02% 🔽
@sentry/browser - ES6 CDN Bundle (minified)	55.5 KB	55.5 KB	0%
@sentry/browser - Webpack (gzipped + minified)	22.6 KB	22.6 KB	0%
@sentry/browser - Webpack (minified)	79.21 KB	79.21 KB	0%
@sentry/react - Webpack (gzipped + minified)	22.63 KB	22.63 KB	0%
@sentry/nextjs Client - Webpack (gzipped + minified)	47.6 KB	47.6 KB	0%
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.36 KB	25.36 KB	0%
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	23.72 KB	23.72 KB	0%

[smeubank]

lookin good 👍 in our daily it came up if you think this would be easily re-used for nextjs?

[AbhiPrasad]

Hey, could you write a README similar to what you did for the playwright tests here?

It can be very WIP - just so we have some of this in code that we can iterate on (also makes future git blames easier instead of digging through PR descriptions)

More fleshed out review incoming

[onurtemizkan]

if you think this would be easily re-used for nextjs?

I think so, it's very similar to the current next server tests after all.

Hey, could you write a README similar to what you did for the playwright tests here?

Sure, will do today. 👍

[lobsterkatie]

The request bodies are matched against jest snapshots. Random or platform-specific properties are patched with expect.any(...). I'm still not sure about this approach as it can be too fragile with even small updates on SDK (especially stack-trace snapshots).

I think it's a valid concern. I also have never liked the fact that snapshots force you to assert on everything, when any given test is probably only testing a subset (and it's a lot harder to tell what subset because of all of the noise). So I try to avoid them if at all possible.

cjs/esm builds are separately tested, but we transform esm builds to cjs beforehand with babel, as jest still doesn't support esm modules natively.

As we discussed this morning (and which I'm including here for posterity), IMHO testing against our built code isn't really something we need to worry about on the node side. With the browser SDK, we're spinning up all sorts of variations when we build bundles (including ones which explicitly delete some of the code), and they build into something barely human-readable (making them really hard to sanity-check), so testing against them gives us a sense of security we wouldn’t get otherwise. None of that applies to Node, though, so I think our collective building, reviewing, and maintenance time is better spent elsewhere.

Having a configurable server script per test will probably make it easier to run and test with databases, but I haven't tried that yet. It would be great to collect some ideas about that as well (My first idea was to have a set of Docker nodes with predefined ports, that we start running before all tests).

Also from our conversation this morning, I think it's safe to mock the DB methods, rather than worrying about trying to spin up live databases. The thing we're trying to test is whether or not we start and stop spans when we should, and whether we're collecting the correct metadata along the way, but not what happens in between. (Put another way, we instrument the call to the DB, not the DB query itself, so as long as we test that, we're good.)

[lobsterkatie]

This is looking good so far!

I wonder, though - is there a way to avoid having the relevant code spread over four separate files (some of which seem pretty boilerplate), with each test having to live in a separate folder? I know that this structure is similar to what we have on the browser side, but I've found with those that I end up doing a whole lot of clicking around just to look at all the pieces of one test. What do you think?

[lobsterkatie]

Following up on my comment above re: snapshots, I think it's enough if we assert on the exception, similar to how Kamil did it in the nextjs integration tests here:

sentry-javascript/packages/nextjs/test/integration/test/client/errorClick.js

Lines 9 to 18 in ba14cc5

	expectEvent(requests.events[0], {
	exception: {
	values: [
	{
	type: 'Error',
	value: 'Sentry Frontend Error',
	},
	],
	},
	});

All we really want to prove is that calling captureException results in an error event being sent. The rest (should) be covered by our unit tests.

[lobsterkatie]

Suggested change

"allowSyntheticDefaultImports": true,

Per the TS docs, turning on esModuleInterop also automatically turns on allowSyntheticDefaultImports.

[lobsterkatie]

Suggested change

"moduleResolution": "node",

This is inherited from @sentry-internal/typescript via the repo's root-level tsconfig.

sentry-javascript/packages/typescript/tsconfig.json

Line 11 in ba14cc5

"moduleResolution": "node",

[lobsterkatie]

Suggested change

"noEmit": false,

This should be the tsc default.

[lobsterkatie]

Suggested change

"strict": true,

This is also inherited:

sentry-javascript/packages/typescript/tsconfig.json

Line 33 in ba14cc5

"strict": true,

[lobsterkatie]

Suggested change

	"types": ["jest"]
	"types": ["jest", "node"]

tsconfig inheritance is unfortunately not additive, so we have to re-specify node here.

[AbhiPrasad]

let's call this suites/public-api/capture-message/... to better match the playwright tests?

[AbhiPrasad]

let's try to match how this is often used by users.

Suggested change

	Sentry.captureException('Captured Error');
	Sentry.captureException(new Error('Captured Error'));

[AbhiPrasad]

this is dangerous because it assumes only 1 item, while envelopes can have multiple items. Can we work around this? See:

sentry-javascript/packages/utils/test/testutils.ts

Lines 15 to 17 in ebc9da5

	export function parseEnvelope(env: string): Array<Record<any, any>> {
	return env.split('\n').map(e => JSON.parse(e));
	}

[AbhiPrasad]

so this starts a new server for each test. Could that get expensive? Could we somehow re-use the same server instance?

[onurtemizkan]

My initial idea was to make servers pluggable just like templates in Playwright tests, so we can write tests for specific server configurations. Would that make sense to check the overall performance and revisit this when we're writing a bit more complex tests?

[AbhiPrasad]

Yeah that’s fine with me - we can def come back and explore further!

[onurtemizkan]

I wonder, though - is there a way to avoid having the relevant code spread over four separate files (some of which seem pretty boilerplate), with each test having to live in a separate folder? I know that this structure is similar to what we have on the browser side, but I've found with those that I end up doing a whole lot of clicking around just to look at all the pieces of one test. What do you think?

I think for Node tests, we might not need separate init and subject scripts, so I merged them as scenarios. Also for server scripts, I added a default as it looks like most of tests can just use that simple express server.

Still not looking great as we still have a separate folder for each test / scenario. The reason for that is when I wrote more than one test in a set, Jest stopped logging the pure error source (like TypeError: Cannot read property 'finish' of undefined). Instead, it started logging a generic "test suite timeout" as it became a test group. I'm not sure if there's a workaround for it, though. I can look for alternative ways too.

[AbhiPrasad]

Still not looking great as we still have a separate folder for each test / scenario

I think this is fine - we can come back and re-evaluate if it's such a pain.

[AbhiPrasad]

@onurtemizkan mind rebasing? Let's get this initial version in and then we can iterate as we go forward.

[AbhiPrasad]

This seems to be from a bad merge commit?

[onurtemizkan]

Oops yes.

[onurtemizkan]

Skipping this test for now, we can re-enable it with the PR to fix #4731.

[lobsterkatie]

Still not looking great as we still have a separate folder for each test / scenario

I think this is fine - we can come back and re-evaluate if it's such a pain.

Taking it down from 4 files to 2 is already def an improvement. My main reasons for asking about this were 1) the test tests the results of whatever happens in the scenario, and it's easier to reason about that cause and effect if you can see them both at once, and 2) (less important) I often find it helpful to just scan down the list of test titles (should xyz when abc) to see what's there, and having one per folder makes that tougher.

I can see why they're currently in two files, though, so jest can run a test file, that test file can call runServer, and runServer can require the scenario file. The only way I could see to get around that (maybe) would be to have runServer take an initSentry function which would live in the test file and do whatever is currently happening in the scenario file. I guess that doesn't account for testing imports, though... IDK if it would be the same to import them in the test file or not. Anyway, I'm fine to merge this as a foundation and tweak it as we go.