Node’s tests should need to be explicit if they intend to test stack traces

[GeoffreyBooth]

I work on modules stuff. If I were to make a change to the module loaders that changes the functions that get called, say by extracting some code from inside a large function into move into its own function, this causes a change in the stack trace for any error thrown by code loaded from that module loader (which is almost all code). This in turn causes 45 tests to fail, for example https://github.com/nodejs/node/blob/main/test/message/source_map_eval.out:

*tabs.coffee:26
	alert "I knew it!"
	^

ReferenceError: alert is not defined
    at Object.eval (*tabs.coffee:26:2)
    at eval (*tabs.coffee:1:14)
    at Object.<anonymous> (*source_map_eval.js:8:1)
    at Module._compile (node:internal/modules/cjs/loader:*)
    at Module._extensions..js (node:internal/modules/cjs/loader:*)
    at Module.load (node:internal/modules/cjs/loader:*)
    at Module._load (node:internal/modules/cjs/loader:*)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:*)
    at node:internal/main/run_main_module:*

Node.js *

To my eye, this test isn’t intending to verify the sequence of functions called before the error is thrown. Perhaps the first few lines are intentional, like at Object.eval (*tabs.coffee:26:2) and at eval (*tabs.coffee:1:14) but presumably nothing beyond that. If we remove the rest of the stack trace from the test, such as by replacing the entire line with * instead of just replacing the line numbers with *, we could remove the CommonJS loader internals stack trace from the scope of this test.

Such a solution would work for any tests where the stack trace is longer than the limit (Error.stackTraceLimit, 10 by default) but it would be brittle for shorter traces; adding a new function to the CommonJS loader that increases the trace by one would cause a broken test.

An alternative would be to add Error.stackTraceLimit = 0 to test/common/index.js. Then stack traces in tests would be empty by default, and tests that intend to test the trace would need to set some value for the stack trace limit as part of the test. So in the example here of https://github.com/nodejs/node/blob/main/test/message/source_map_eval.js, assuming we want to test the first three lines of this trace then Error.stackTraceLimit = 3 could be set as part of the test.

Making this change would involve updating all the affected tests, but after that we should be free to refactor the module loaders without breaking too-specific tests. The Error.stackTraceLimit = 0 default would also ensure that future contributors who aren’t aware of this “don’t test more than you need to in the stack trace” rule wouldn’t be breaking it through ignorance; if they deliberately want to test the trace, they could read something somewhere (the contributors’ guide?) to learn about the need to set Error.stackTraceLimit as part of their test. @nodejs/testing

[aduh95]

An alternative would be to add Error.stackTraceLimit = 0 to test/common/index.js.

I don't think that's a good idea, for most tests having a stack trace is actually useful for debugging, I'm -1 on making worse all the tests for a problem that only affects test/message.

Why not adding Error.stackTraceLimit = 3 to all the test/message tests that break with by the next loader change instead?

[GeoffreyBooth]

Why not adding Error.stackTraceLimit = 3 to all the test/message tests that break with by the next loader change instead?

Wouldn't it be inevitable that someone will later on add a new message test and forget to add Error.stackTraceLimit = 3? That was the appeal of making the restricted trace the default that requires intention to override, rather than an exception.

[targos]

I'd be ok with a default restricted trace for message tests only.

[GeoffreyBooth]

I'd be ok with a default restricted trace for message tests only.

Is there the equivalent of common/index.js for just the message tests?