Upgrade tap to v18

[bizob2828]

Description

We're currently using v16 of tap. v18 is a significant rewrite. I took a look at what it would take and we have to replace t.autoend() with t.end() at the end of every suite. Also adding custom assertions is no longer via addAssert.

Acceptance Criteria

node-newrelic tests are run with tap 18.

Additional context

tap 18 requires you to write plugins for any custom work. It's very heavy handed but in the near term to add custom assertions we're just adding them on tap.Test.prototype

[workato-integration[bot]]

https://new-relic.atlassian.net/browse/NR-218084

[jsumners-nr]

@bizob2828 what are your thoughts on not attempting to add custom assertions and instead use utility functions, e.g.?

function validateFoo(t, foo) {
  t.equal(foo, 'foo')
}

tap.test('foo', async t => {
  validateFoo(t, 'foo')
})

[bizob2828]

We could. That's how most of these once were but opted for custom assertions.

[jsumners-nr]

I think doing so would solve a couple problems:

1. If the test suite's API changes again in the future, we don't get surprised by it.
2. If for some reason we decide to change test suites then we are loosely coupled and ready to make the switch.
3. Less mystery in what is available. I find custom assertions to be non-discoverable and typically only known about through rumor and conjecture.

[bizob2828]

I'll prob do this in a separate PR as the changes for 18 are a lot

[bizob2828]

If for some reason we decide to change test suites then we are loosely coupled and ready to make the switch.

We still have tap assertions within the helper so not sure how loosely coupled we are

[bizob2828]

I'm putting into the backlog for now.

Branch

I have one major blocker which is detailed here.

Aside from that we have a failing integration test test/integration/core/exceptions.tap.js, the IPC communication between the test and the test harness in a child process is emitting too many of the same events. I think this is because tap is hooking into process.emit. Lastly, we may not be able to use tap 18 because it's so much slower than 16.

[isaacs]

Less mystery in what is available. I find custom assertions to be non-discoverable and typically only known about through rumor and conjecture.

For what it's worth, while it is a bit more "heavy", the plugin approach in tap v18 is specifically designed to make the system type-aware, so that autocomplete can surface any functionality added by plugins for better discovery (assuming that the plugin exports types, of course.)

we may not be able to use tap 18 because it's so much slower than 16.

I'm curious about this. What is slower? How much is "so much"?

[bizob2828]

Less mystery in what is available. I find custom assertions to be non-discoverable and typically only known about through rumor and conjecture.

For what it's worth, while it is a bit more "heavy", the plugin approach in tap v18 is specifically designed to make the system type-aware, so that autocomplete can surface any functionality added by plugins for better discovery (assuming that the plugin exports types, of course.)

@isaacs I find plugins for internal custom assertions where we don't use typescript/types to be very heavy handed. It also seems to require the use of a monorepo, or separate published modules for the plugins. Maybe I'm not grasping the concept but it wan't clear to me.

we may not be able to use tap 18 because it's so much slower than 16.

I'm curious about this. What is slower? How much is "so much"?

I'm not sure if it's the reporter or what but we used to rely on the classic reporter and you can see unit test runs in tap 16 were finishing in 1 min. With tap 18 it's taking almost 4 minutes. This is a deal breaker for us and I don't know where to begin to give you enough information to log an issue, any suggestions?

[jsumners-nr]

For what it's worth, while it is a bit more "heavy", the plugin approach in tap v18 is specifically designed to make the system type-aware, so that autocomplete can surface any functionality added by plugins for better discovery (assuming that the plugin exports types, of course.)

For clarity, I'm not at all concerned with autocomplete functionality. My concern is encountering methods on the library that I don't expect and having to figure out how they got defined. One can argue that the path to learning where the method is defined and how it works is the same and learning where a function is defined, but I personally find it easier to reason through standalone function than one that probably relies on internals of another thing I don't know.

[isaacs]

@isaacs I find plugins for internal custom assertions where we don't use typescript/types to be very heavy handed. It also seems to require the use of a monorepo, or separate published modules for the plugins. Maybe I'm not grasping the concept but it wan't clear to me.

Yes, plugins are dependencies. Depending on your view of how "heavy" a thing must be to justify publishing to npm (which can be quite a low bar indeed), that can be either heavy or light.

I'm not sure if it's the reporter or what but we used to rely on the classic reporter and you can see unit test runs in tap 16 were finishing in 1 min. With tap 18 it's taking almost 4 minutes. This is a deal breaker for us and I don't know where to begin to give you enough information to log an issue, any suggestions?

Yeah, 4 minutes is a lot. Since you're not using typescript, I'd maybe see if tap plugin rm @tapjs/typescript or tap config set typecheck=false makes it go faster. Ts-node can be pretty slow, especially if you are using typescript with allowJs: true in tsconfig. This will be improved somewhat once I finish combing through the nest of edge cases to get tsimp stable enough for prime time.

For clarity, I'm not at all concerned with autocomplete functionality.

Ah, a fellow old, I see ;) Yeah, kids these days don't read docs or source anymore. They just press tab in their editor and expect it to be correct. When that fails, ask copilot or chatgpt. (All jokes aside, "types as inline docs" is actually a pretty nice pattern, I've found myself leaning on it more and more, and appreciating when a library has accurate types included, for that reason.)

[bizob2828]

@isaacs I tried tap plugin rm @tapjs/typescript and it didn't affect the runtime of unit tests

In CI:

tap 16 unit tests - ~1 min 16s tap 18 unit tests stock - ~3m 30s tap 18 unit tests with typescript plugin removed - ~3m 30s

[isaacs]

@bizob2828 That's very bizarre. Can you reproduce it locally? Is it doing a build before running the tests, or are the tests themselves slower? Maybe it's defaulting to a lower concurrency value?

[bizob2828]

@bizob2828 That's very bizarre. Can you reproduce it locally? Is it doing a build before running the tests, or are the tests themselves slower? Maybe it's defaulting to a lower concurrency value?

No building. We are using c8 for coverage and disabling tap c8. Reason is we have to pass in an option to c8 and couldn't figure out how to do that through tap.

Dumping config between 16 and 18 it has jobs set to 8 so concurrency is the same. A quick check for you would be to clone this repo and just run

npm ci
npm run unit

That's with tap 16. Then with tap 18 it'd be to checkout this branch in fork and run the same 2 commands

[bizob2828]

I rebased and upgraded tap to 19. Same issues still exist from here but a new update is we have some unit tests failing because of this tap issue. Test runs are still very slow so i'm pausing

[isaacs]

@bizob2828 You know, if you're using c8 directly, that might also be contributing to the slowness.

Tap no longer uses c8 for coverage, only for reporting. For instrumenting coverage, it's using @tapjs/processinfo, since that allows us to only instrument the code that ought to be covered, instead of capturing coverage for everything that gets loaded and dumping a gb of json that's 99% irrelevant. (These numbers aren't exaggerations, they're typical of a mid- to large-sized project, where the vast majority of the code being run is in node_modules and node internals.)

What c8 option are you setting? Maybe it's something that could be built into processinfo.

[bizob2828]

@isaacs I'll look again but my test times were running without c8 and without coverage from tap. We're using this --merge-async flag to c8 as our coverage reports were causing Node.js to OOM

[bizob2828]

@isaacs if you use this branch you can see the times without coverage:

npm run unit:no-cov

time:

real    1m9.855s
user    3m54.363s
sys 1m9.708s

Then with tap 16 and c8 coverage

git checkout main
npm i 
npm run unit

time:

real    0m35.679s
user    1m41.781s
sys 0m37.934s

[bizob2828]

We have decided to bail on using tap. Instead, we're going to incrementally migrate to native test runner. I'm closing this issue

[isaacs]

Since speed is a major factor for you, the native node:test is probably the way to go. It's maybe less ergonomic or featureful, but it definitely is fast.