[DI] Switch unit tests to Mocha instead of Tap

[watson]

The reason for the change are that Tap has several bugs and drawbacks:

• It throws nasty native stack trace when assertions fail in certain async situations.
• Doesn't show nice diff of expected vs actual data in deeply nested object assertions.
• No support for .only when using the Mocha-syntax variant.
• Output from console.logs interfere with the Tap output. It's often interweaved, cut off i the middle of a string or mis-colored (probably because of multi-threading).

[watson]

• Added foo property to probe result #4757
• [DI] Add ability to take state snapshot #4549
• [DI] Switch unit tests to Mocha instead of Tap #4728 👈
• [DI] Add stack trace to log probe results #4727 : 1 other dependent PR (#4729 )
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @watson and the rest of your teammates on Graphite

[github-actions]

Overall package size

Self size: 7.21 MB
Deduped: 62.61 MB
No deduping: 62.89 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.1.1 | 18.67 MB | 18.68 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.4.1 | 2.14 MB | 2.23 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | jsonpath-plus | 9.0.0 | 580.4 kB | 1.03 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | lru-cache | 7.14.0 | 74.95 kB | 74.95 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

[pr-commenter]

Benchmarks

Benchmark execution time: 2024-10-02 10:32:32

Comparing candidate commit 9a5d691 in PR branch watson/DEBUG-2605/mocha with baseline commit 4d2f5b8 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 6 unstable metrics.

[juan-fernandez]

(absolute nit) unrelated to this PR: using :debugger is a bit confusing to me given that debugger is a keyword and that I usually use test:debug to pass --inspect-brk to test sessions.

Maybe test:dynamic-inst ? That's awful 😆 , so feel free to disregard

[watson]

I would have preferred to use something related to our product name as well, but it's called "debugger" in all the other languages for historical reasons, so I was told to use this name in Node.js as well. I'll ask around to see if we still think this is a good idea. But if we decide to rename it, I'll make a separate PR as it touches a lot of other areas of the code as well - not just these lines.

[rochdev]

We've been switching in the other direction, so not sure this would actually be productive. I'm pretty sure most if not all of these issues are because of how we use it too, and not because of tap itself. There are some huge benefits to tap that Mocha cannot do, like running tests in parallel and in isolation (Mocha does parallel but doesn't isolate the tests), and being able to run a single test file directly.

@watson Did you try to use the test module instead? Last time I tried it it worked pretty well, the only reason I didn't make the switch is that there were tons of issues with our own tests, but since for you it's a fresh test suite that is only a few weeks old, maybe it would be easier to port. Ideally, eventually we'd move everything to test (and subsequently to the built-in node:test once the features we need are available in all Node versions we support).

[watson]

Did you try to use the test module instead?

I did not. I thought we were married to the mocha style. But I'm happy to hear we're not and you're right many of the issues I have discovered are because we're using the mocha syntax. I'll try to switch and see if it fixes all the mentioned issues.

[watson]

I'm at a bit of an impasse. We currently depend on an old version of tap, and re-writing these tests using that old version gives me a bunch of problems. It would be easier for me to wait using tap until we have upgraded to this new version which has support for using sinon. So since the DI tests doesn't currently work well with tap (and it's only going to get worse in upcoming PR's were I add more test), I'd like to stop using it until we upgrade the version of tap we use in the project.

[rochdev]

I did not. I thought we were married to the mocha style. But I'm happy to hear we're not and you're right many of the issues I have discovered are because we're using the mocha syntax. I'll try to switch and see if it fixes all the mentioned issues.

We're not married to any style per se, but there are currently 2 main reasons for the Mocha style:

1. We've used Mocha historically, so all tests are currently written in that format.
2. Perhaps related to 1, having one consistent syntax to write tests definitely makes contributing to multiple components of the library easier, and since everything else is in Mocha, it only makes sense to continue with that.

If we can agree on a different format that will be adopted by everyone going forward, I think we could change the syntax and then eventually rewrite the old tests with the new syntax. At that point I'd probably go with the syntax of the built-in runner even if that means we need some polyfill since it will then be easier to move to that, and minimize our use of libraries to minimize dependencies. But all of that is definitely out of the scope of this PR.

The base branch was changed.