Discussion/Tracking: Adding more structure to Node.js' tests

[jasnell]

It's no secret at this point that other runtimes (Deno, Bun, Workers, etc) are seeking to improve compatibility with Node.js. In order to accomplish this, these other runtimes need the ability to run Node.js' tests since those are really the closet thing we have to a conformance test suite. The challenge, however, is that the current corpus of tests are largely unstructured blobs of code that mix tests for public API with tests for internal API using a bespoke test harness that relies on Node.js specific characteristics. This makes it difficult for runtimes to partially/selectively implement Node.js compatibility and generally forces these other runtimes to manually port tests to work in those environments... particularly when the tests depend on specific Node.js internals that are not necessarily part of the public API surface (e.g. any test that relies on --expose-internals)

This issue serves two purposes: 1) as a place where we can discuss and try to come to consensus on a way of improving the structure of our tests, and 2) a tracking issue to track the progress of such activity.

There are four key issues:

1. Mixing of internal and public APIs in tests without clear delineations between those.
2. Lack of consistent documentation/comments explaining exactly what is being tested at any given point. This includes lack of documentation around what the expected behaviors being asserted are, etc.
3. Haphazard organization. Some of the test files haphazardly just kind of sprinkle individual tests throughout the file in an almost organic or ad hoc way.
4. Use of the bespoke test harness (require('../common') and it's reliance on various internal, undocumented behaviors and features that may be difficult to reconcile to other environments.

To improve overall structure of the tests, I propose:

1. ... that we begin making use of our own node:test internal module to impose a more logical structure to the tests. I don't think it matters if that is using either the test('...', () => { ... }) or BDD describe('...', () => { ... }) pattern. I've seen both in use in a few tests already and some folks seem to prefer the BDD pattern. I'm good with that.

2. ... that we reduce dependency on our internal bespoke test harness. For instance, common.mustCall() can be replaced (albeit with a bit more verbosity) with mock.fn() checks. The current ../common.js can be decomposed into individual imports to allow it to be more incrementally applied. The harness can also be made a bit less specific to Node.js and possibly even published as a separate module that can be made to run on multiple runtimes.

3. ... that we begin asking for tests to include more documentation/comments on what is being tested, what the expected behaviors are, what the edge cases are, etc. With more structure using the BDD model that likely comes more naturally and we might not need more comments in the tests, but there are still nuances that are worthy of improved documentation.

4. ... that we separate tests verifying internal behaviors from tests verifying external behavior.

Ultimately I'd like us to get to a point where we have a clearly delineated set of public API tests that other runtimes can pick up almost as is and run in their environments... a lot like we can do with the Web Platform Tests. These would be separated from the internal tests that verify specific aspects of Node.js' internals that aren't considered public API and wouldn't really be expected of other runtimes to implement in exactly the same way. To that end, I would propose eventually splitting the current test/parallel directory into two separate folders, test/parallel/public and test/parallel/internal, with the test/parallel/public being the primary set of tests other runtimes should seek to run and align with. Likewise, we'd have test/sequential/public and test/sequential/internal ... or, we can flip it around and have test/public/parallel, test/public/sequential, test/internal/parallel, test/internal/sequential. The key differentiator is that nothing in test/public would be allowed to use things like --expose-internals and would be set up only to set the documented public API surface.

Adding additional structure here obviously increases dev friction a bit, so all this needs to be carefully considered.

This is no small task. There are many thousands of individual test files that would need to be updated. So before jumping into this whole process in earnest, let's chat about it and gets some input from the contributor base.

@nodejs/tsc, @nodejs/collaborators, @nodejs/testing

[cjihrig]

For instance, common.mustCall() can be replaced (albeit with a bit more verbosity) with mock.fn() checks. The current ../common.js can be decomposed into individual imports to allow it to be more incrementally applied.

Not to sidetrack the issue, but I would welcome some of the features like common.mustCall() in node:test.

[jasnell]

common.mustCall() can be covered by use of mock.fn() ... it's a bit verbose right now but possible. The key thing that trips up about common.mustCall() is that not every runtime actually supports a mechanism like process.on('exit', ...) where those are checked. If it could be scoped to the individual test (e.g. test('...', () => { ... }) then it would be a lot easier.

[avivkeller]

To that end, I would propose eventually splitting the current test/parallel directory into two separate folders

I've proposed #53640, which would allow the test runner to be able to pick up those subdirectories. I closed (and now reopened) it as it didn't reach consensus, but this issue may be a reason to reconsider it. (in a new light)

---

Overall, I'm so excited for this to coming :-)

[cjihrig]

common.mustCall() can be covered by use of mock.fn() ... it's a bit verbose right now but possible. The key thing that trips up about common.mustCall() is that not every runtime actually supports a mechanism like process.on('exit', ...) where those are checked. If it could be scoped to the individual test (e.g. test('...', () => { ... }) then it would be a lot easier.

I think mustCall() would be fairly straightforward to implement. We already have mocks scoped to individual tests that are restored when the test completes. I think we would want to add a new option to mock.fn(). That option would be a callback function that runs when the mock is restored. That callback would receive the mock context as an argument, and could perform assertions. mustCall() would be sugar that leverages that callback.

I believe this is a minimal implementation of `t.assert.mustCall()` if the callback I mentioned above existed.

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index 2672a25b61..9104cad524 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -122,6 +122,14 @@ function lazyAssertObject(harness) {
       assertObj.set(methodsToCopy[i], assert[methodsToCopy[i]]);
     }
 
+    assertObj.set('mustCall', function(fn, times) {
+      return this.mock.fn(fn, {
+        onRestore(mock) {
+          assert.strictEqual(mock.calls.length, times);
+        }
+      });
+    });
+
     const { getOptionValue } = require('internal/options');
     if (getOptionValue('--experimental-test-snapshots')) {
       const { SnapshotManager } = require('internal/test_runner/snapshot');

[cclauss]

My comment at #53640 (comment) clearly slowed this process. I apologize for that. That comment was focused on Python tests only. Our modifications to Python tests should remove the custom test discovery code and instead rely on the automated test discovery conventions that are broadly used in other massive projects.

There concerns are unrelated to non-Python tests.

[targos]

I am in favor of the general idea, but not really convinced by the separation between public and private APIs in the folder structure.

[lpinca]

1. ... that we begin making use of our own node:test internal module to impose a more logical structure to the tests

There are tests with subtests and in this case using node:test and more structure might make sense, but there are also tests where using node:test would only be unnecessary added complexity. An as example take the following test (parallel/test-sys.js)

'use strict';
require('../common');
const assert = require('assert');
const sys = require('sys');
const util = require('util');

assert.strictEqual(sys, util);

Rewriting it using node:test would result in something like this

'use strict';
require('../common');
const test = require('node:test');
const assert = require('assert');
const sys = require('sys');
const util = require('util');

test('sys was renamed to util', (t) => {
  assert.strictEqual(sys, util);
});

What do we gain from this? The test now uses unnecessary modules like async_hooks, runs slower, and it is still not portable as it uses a non-standard test syntax.

I think this whole effort is more beneficial for other runtimes than Node.js itself.

[LiviaMedeiros]

About the splitting tests into subdirectories: are symlinks an option here? Roughly,

test/group/by-subsystem/util/test-util-internal.js -> test/parallel/test-util-internal.js
test/group/by-visibility/uses-internals/test-util-internal.js -> test/parallel/test-util-internal.js

So we can avoid breaking all scripts and habits depending on test/parallel/*.{js,mjs}, and allow folks to work with smaller logical groups at the same time.

[H4ad]

What do we gain from this? The test now uses unnecessary modules like async_hooks, runs slower, and it is still not portable as it uses a non-standard test syntax.

By just reading the code, I can understand why this test exist with the second example but not with the first example by just looking at the code, and the complexity was just +3 lines of code.

To that end, I would propose eventually splitting the current test/parallel directory into two separate folders, test/parallel/public and test/parallel/internal, with the test/parallel/public

I think we can keep the public tests in the same folder today, and only move the internal ones just to not loose the history of changes of that file.

Also, maybe just renaming the test to have .internal or -internal will be better because of the scripts the @LiviaMedeiros mentioned.

[lpinca]

and the complexity was just +3 lines of code

Requiring node:test is not just +3 lines of code.

[mcollina]

While I favor improving our test suite, I wonder if making it easier for other implementations should be one of our project goals. As stated, providing a clear definition of Node.js would directly benefit Workers, Deno, and Bun.

I'm unsure if definiting a "Node.js Compatibility Kit" should be part of our strategy and if volunteers would contribute/be interested in it. However, we have to talk about it because Node.js is the de-facto standard, and CloudFlare, Deno and Oven (Bun's company) are lifting our tests and part of our codebases anyway. Therefore, taking control of how another runtime can claim to be "Node.js Compatible" is worthwhile and it should attract funding and contributions from said companies.

This could be seen as a major strategic initiative. Is CloudFlare going to sponsor/fund this work?

---

I don't see how the technical details matter at this point, because if we decide we want to have a Node.js Compatibility Kit, any technical roadblocks could be removed. Nevertheless, here is my take: I don't see how to move all tests to node:test. node:test customizes the running environment to run the tests. For example, it adds an AsyncLocalStorage and a few other key changes, likely making it hard to test async_hooks and other public APIs. In other cases, a change in the underlining libraries used by node:test would prevent the test from running, making it really hard to debug what's going on.

[benjamingr]

I'm -1 on any changes that make Node tests worse in order to make it easier for other runtimes to be compatible with its APIs.

Node.js APIs are either web standards or they're Node's APIs, we only make guarantees to our users through the stability index and release cycle.

If someone wants a runtime compatible with Node.js in its entirety - they should use Node.js. If they want interoperable code instead they should build on web standards.

That said: anything that generally improves Node's tests I'm +1 on. I just don't think compatibility of our non-standard APIs should be a goal.