update some tests to run in isolated workers using tap #2538

rochdev · 2022-11-17T01:47:29Z

What does this PR do?

Update some tests to run in isolated workers using tap.

Motivation

We need a way to isolate tests so that we can write integration tests without complex logic to cleanup after the suite, which is also not always possible.

codecov · 2022-11-17T01:48:29Z

Codecov Report

Merging #2538 (8f9d9a9) into master (9b350c1) will decrease coverage by 2.46%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2538      +/-   ##
==========================================
- Coverage   89.74%   87.29%   -2.46%     
==========================================
  Files         291      294       +3     
  Lines        9958     9996      +38     
==========================================
- Hits         8937     8726     -211     
- Misses       1021     1270     +249

Impacted Files	Coverage Δ
packages/dd-trace/src/appsec/reporter.js	`12.30% <0.00%> (-87.70%)`	⬇️
packages/dd-trace/src/appsec/index.js	`17.24% <0.00%> (-82.76%)`	⬇️
...kages/dd-trace/src/appsec/gateway/engine/runner.js	`17.64% <0.00%> (-82.36%)`	⬇️
...kages/dd-trace/src/appsec/gateway/engine/engine.js	`27.11% <0.00%> (-72.89%)`	⬇️
...ckages/dd-trace/src/appsec/gateway/engine/index.js	`27.77% <0.00%> (-72.23%)`	⬇️
...trace/src/appsec/iast/taint-tracking/operations.js	`32.81% <0.00%> (-64.07%)`	⬇️
packages/dd-trace/src/appsec/rule_manager.js	`36.36% <0.00%> (-63.64%)`	⬇️
...ce/src/appsec/iast/analyzers/injection-analyzer.js	`62.50% <0.00%> (-37.50%)`	⬇️
.../dd-trace/src/appsec/iast/taint-tracking/filter.js	`66.66% <0.00%> (-33.34%)`	⬇️
.../src/appsec/iast/analyzers/weak-cipher-analyzer.js	`66.66% <0.00%> (-33.34%)`	⬇️
... and 34 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pr-commenter · 2022-11-19T02:31:28Z

Benchmarks

Comparing candidate commit 8f9d9a9 in PR branch mocha-parallel with baseline commit 9b350c1 in branch master.

Found 1 performance improvements and 14 performance regressions! Performance is the same for 951 cases.

scenario:plugin-bluebird-with-tracer-14

🟥 cpu_user_time [+5.797ms; +8.801ms] or [+2.502%; +3.799%]
🟥 execution_time [+8.188ms; +8.945ms] or [+3.142%; +3.432%]
🟥 max_rss_usage [+0.935KB; +1.023KB] or [+1.797%; +1.965%]

scenario:plugin-http-server-with-tracer-14

🟩 cpu_usage_percentage [-2.035%; -1.426%]
🟥 execution_time [+33.964ms; +48.168ms] or [+4.054%; +5.750%]

scenario:startup-control-everything-14

🟥 instructions [+54325848; +63885594] or [+1.732%; +2.037%]
🟥 max_rss_usage [+3.370KB; +5.827KB] or [+2.344%; +4.053%]

scenario:startup-with-tracer-everything-14

🟥 instructions [+66801126; +77173940] or [+1.847%; +2.134%]

scenario:plugin-bluebird-with-tracer-16

🟥 cpu_user_time [+5.843ms; +9.294ms] or [+2.487%; +3.956%]
🟥 execution_time [+8.309ms; +8.881ms] or [+3.125%; +3.340%]

scenario:startup-control-everything-16

🟥 instructions [+67377175; +77670469] or [+2.136%; +2.462%]

scenario:startup-with-tracer-everything-16

🟥 instructions [+64965020; +77274217] or [+1.813%; +2.157%]

scenario:startup-control-everything-18

🟥 instructions [+58268056; +69357639] or [+1.936%; +2.305%]

scenario:startup-with-tracer-everything-18

🟥 cpu_user_time [+0.021s; +0.047s] or [+1.508%; +3.369%]
🟥 instructions [+75478357; +87202152] or [+2.180%; +2.519%]

bengl · 2022-11-22T21:28:21Z

packages/dd-trace/test/appsec/iast/overhead-controller.spec.js

@@ -402,7 +404,7 @@ describe('Overhead controller', () => {
        })
      }

-      testInRequest(app, tests)
+      () => testInRequest(app, tests) // TODO: re-enable when fixed with tap


A bare arrow function is a pretty weird way to comment something out. Can you just use a normal comment here?

Sure, I was trying to avoid changing too many lines and came up with this, but I can comment out all the lines involved instead.

Can't you just comment out this line?

packages/dd-trace/test/profiling/exporters/agent.spec.js

Qard · 2022-11-23T02:25:07Z

Lots of notes about things not working with this test runner. Is the plan to fix these before landing? Otherwise we should at least elaborate on why those tests are not working so it's clear what is involved in enabling them again.

simon-id

Are you sure we can't find a way to include the setup script automatically for every file ? using the NODE_ARGS env var with a -r or something ?

simon-id · 2022-11-23T10:30:38Z

packages/dd-trace/test/appsec/iast/path-line.spec.js

+    // TODO: make this work regardless of the test runner
+    it.skip('call does not fail', () => {


cc: @uurien

packages/dd-trace/test/exporters/scheduler.spec.js

.github/workflows/appsec.yml

packages/dd-trace/test/appsec/iast/analyzers/command-injection-analyzer.spec.js

…-analyzer.spec.js Co-authored-by: simon-id <simon.id@datadoghq.com>

simon-id · 2022-11-24T09:33:20Z

@uurien The CI is failing on windows for IAST tests only, please take a look.

packages/dd-trace/test/profiling/exporters/agent.spec.js

Qard · 2022-11-25T18:50:37Z

@simon-id I would prefer we not use extra args to do that. I'd like to be able just run a single test directly more easily by just running node path/to/test.js.

packages/dd-trace/test/appsec/iast/overhead-controller.spec.js

tlhunter · 2023-02-22T21:05:28Z

packages/datadog-core/test/storage/async_hooks.spec.js

@@ -1,5 +1,7 @@
 'use strict'

+require('../setup')


Should these first two files be require('../setup/tap')?

rochdev · 2023-03-13T21:01:33Z

Superseded by #2889

update mocha tests to run in parallel

80f017d

rochdev added the semver-patch label Nov 17, 2022

rochdev added 5 commits November 16, 2022 21:01

remove reporters not supporting parallel mode

f2148f8

increase timeout and move mocha common options to file

2001a15

increase timeout

5290e4b

switch to tap for some of the tests

b3f1567

fix 3rd party license

fd37c71

rochdev added 6 commits November 18, 2022 21:37

move appsec to a separate test run

c4baef4

fix tests on windows

7c1941b

fix unix-only test

230c5c1

remove tap timeout config

96763a6

fix test setup in mocha

2e198df

fix instrumentation tests

4eaa0a1

rochdev changed the title ~~update mocha tests to run in parallel~~ update some tests to run in isolated workers using tap Nov 19, 2022

rochdev marked this pull request as ready for review November 22, 2022 21:24

rochdev requested a review from a team as a code owner November 22, 2022 21:24

bengl requested changes Nov 22, 2022

View reviewed changes

comment out entire test instead of just line calling it

b0283b8

simon-id requested changes Nov 23, 2022

View reviewed changes

rochdev added 2 commits November 23, 2022 10:46

Merge branch 'master' into mocha-parallel

d5d0ec0

move appsec to tap

8bf4500

simon-id reviewed Nov 23, 2022

View reviewed changes

.github/workflows/appsec.yml Show resolved Hide resolved

packages/dd-trace/test/appsec/iast/analyzers/command-injection-analyzer.spec.js Show resolved Hide resolved

Update packages/dd-trace/test/appsec/iast/analyzers/command-injection…

43799f8

…-analyzer.spec.js Co-authored-by: simon-id <simon.id@datadoghq.com>

simon-id reviewed Nov 24, 2022

View reviewed changes

packages/dd-trace/test/profiling/exporters/agent.spec.js Show resolved Hide resolved

uurien and others added 2 commits November 24, 2022 21:07

Fix IAST windows test (hopefully)

27ae39d

Merge branch 'master' into mocha-parallel

3589322

tlhunter reviewed Dec 1, 2022

View reviewed changes

packages/dd-trace/test/appsec/iast/overhead-controller.spec.js Show resolved Hide resolved

rochdev added 2 commits December 6, 2022 18:04

skip using tap feature instead of commenting the test

830466e

fix skip method outside of tap

8f9d9a9

tlhunter reviewed Feb 22, 2023

View reviewed changes

rochdev mentioned this pull request Mar 13, 2023

switch to tap for ci/core/tracing/profiling tests #2889

Merged

rochdev closed this Mar 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update some tests to run in isolated workers using tap #2538

update some tests to run in isolated workers using tap #2538

rochdev commented Nov 17, 2022 •

edited

Loading

codecov bot commented Nov 17, 2022 •

edited

Loading

pr-commenter bot commented Nov 19, 2022 •

edited

Loading

bengl Nov 22, 2022

rochdev Nov 22, 2022

bengl Nov 22, 2022

rochdev Nov 22, 2022

Qard commented Nov 23, 2022

simon-id left a comment

simon-id Nov 23, 2022

simon-id commented Nov 24, 2022

Qard commented Nov 25, 2022

tlhunter Feb 22, 2023

rochdev commented Mar 13, 2023

		// TODO: make this work regardless of the test runner
		it.skip('call does not fail', () => {

update some tests to run in isolated workers using tap #2538

update some tests to run in isolated workers using tap #2538

Conversation

rochdev commented Nov 17, 2022 • edited Loading

What does this PR do?

Motivation

codecov bot commented Nov 17, 2022 • edited Loading

Codecov Report

pr-commenter bot commented Nov 19, 2022 • edited Loading

Benchmarks

scenario:plugin-bluebird-with-tracer-14

scenario:plugin-http-server-with-tracer-14

scenario:startup-control-everything-14

scenario:startup-with-tracer-everything-14

scenario:plugin-bluebird-with-tracer-16

scenario:startup-control-everything-16

scenario:startup-with-tracer-everything-16

scenario:startup-control-everything-18

scenario:startup-with-tracer-everything-18

bengl Nov 22, 2022

Choose a reason for hiding this comment

rochdev Nov 22, 2022

Choose a reason for hiding this comment

bengl Nov 22, 2022

Choose a reason for hiding this comment

rochdev Nov 22, 2022

Choose a reason for hiding this comment

Qard commented Nov 23, 2022

simon-id left a comment

Choose a reason for hiding this comment

simon-id Nov 23, 2022

Choose a reason for hiding this comment

simon-id commented Nov 24, 2022

Qard commented Nov 25, 2022

tlhunter Feb 22, 2023

Choose a reason for hiding this comment

rochdev commented Mar 13, 2023

rochdev commented Nov 17, 2022 •

edited

Loading

codecov bot commented Nov 17, 2022 •

edited

Loading

pr-commenter bot commented Nov 19, 2022 •

edited

Loading