benchmark: more tests to confirm benchmark code isn't broken by Trott · Pull Request #14951 · nodejs/node

Trott · 2017-08-20T04:31:16Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test benchmark

targos · 2017-08-20T06:49:36Z

test/parallel/test-benchmark-events.js

That file is still a copy of the test-benchmark-domain

Yikes, fixed, thanks.

refack · 2017-08-20T17:07:23Z

test/parallel/test-benchmark-events.js

Fixed, thanks!

refack · 2017-08-20T17:34:20Z

test/parallel/test-benchmark-path.js

You can add:

'--set', 'props=',

to further reduce iterations

The two benchmarks that have props each seem to have a single entry only. Are you suggesting adding props= to future-proof things in case more entries are added later to the benchmarks? (Seems like a good idea to me. Making sure I'm not misunderstanding your suggestion.)

Went and added it. Thanks.

Add minimal test to confirm that domain benchmarks run.

Add minimal test to confirm that events benchmarks run.

Add minimal test to confirm that os benchmarks run.

Add minimal test to confirm that path benchmarks run.

Add minimal test to confirm that process benchmarks run.

Trott · 2017-08-24T05:45:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9820/

maclover7 · 2017-08-24T06:09:51Z

Is it worth it to create an internal common.runBenchmark-type function to abstract away most of this logic? It seems like the majority of each test file is shared/duplicated, with the exception of setting a few options.

Trott · 2017-08-24T06:28:56Z

Is it worth it to create an internal common.runBenchmark-type function to abstract away most of this logic? It seems like the majority of each test file is shared/duplicated, with the exception of setting a few options.

A case could certainly be made. I'm generally resistant to adding more functions to common/index.js unless it's something that will be truly ubiquitous in our tests. But a benchmark-testing specific module in the common directory is something I could get behind.

maclover7 · 2017-08-24T06:41:26Z

But a benchmark-testing specific module in the common directory is something I could get behind.

Agree -- was envisioning something similar to the way the DNS tests have a separate module for their shared logic.

Is it best to open up a new PR for this change (before/after this PR is merged?) to further discuss the idea, or for the change to go along with this one?

Trott · 2017-08-24T06:45:18Z

@maclover7 I'd say separate PR. I'm literally seconds away from landing this one. :-D

(I assume this is something you want to tackle.)

Add minimal test to confirm that domain benchmarks run. PR-URL: nodejs#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that events benchmarks run. PR-URL: nodejs#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that os benchmarks run. PR-URL: nodejs#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that path benchmarks run. PR-URL: nodejs#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that process benchmarks run. PR-URL: nodejs#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott · 2017-08-24T06:45:48Z

Landed in 3fcfca6, afa62a2,, dd60c60, and ce69d01 and 3cf27f1

Add minimal test to confirm that domain benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that events benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that os benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that path benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that process benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that domain benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that events benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that os benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that path benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that process benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that domain benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that events benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that os benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that path benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that process benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that domain benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that events benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that os benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that path benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Add minimal test to confirm that process benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2017-09-20T17:35:13Z

@Trott all of these rely on benchmark having run.js

Should we backport this?

Trott · 2017-09-25T04:29:15Z

Should we backport this?

Not worth it IMO. I'll add dont-land labels.

Trott added benchmark Issues and PRs related to the benchmark subsystem. test Issues and PRs related to the tests. labels Aug 20, 2017

targos reviewed Aug 20, 2017

View reviewed changes

cjihrig approved these changes Aug 20, 2017

View reviewed changes

jasnell approved these changes Aug 20, 2017

View reviewed changes

refack reviewed Aug 20, 2017

View reviewed changes

Trott added 5 commits August 23, 2017 22:36

test: add test-benchmark-domain

848997f

Add minimal test to confirm that domain benchmarks run.

test: add test-benchmark-events

c83090a

Add minimal test to confirm that events benchmarks run.

test: add test-benchmark-os

38ee69a

Add minimal test to confirm that os benchmarks run.

test: add test-benchmark-path

c5318fe

Add minimal test to confirm that path benchmarks run.

test: add test-benchmark-process

3899126

Add minimal test to confirm that process benchmarks run.

Trott force-pushed the test-benchmark-domain branch from 09b239d to 3899126 Compare August 24, 2017 05:44

Trott added a commit to Trott/io.js that referenced this pull request Aug 24, 2017

test: add test-benchmark-domain

3fcfca6

Add minimal test to confirm that domain benchmarks run. PR-URL: nodejs#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott added a commit to Trott/io.js that referenced this pull request Aug 24, 2017

test: add test-benchmark-events

afa62a2

Add minimal test to confirm that events benchmarks run. PR-URL: nodejs#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott added a commit to Trott/io.js that referenced this pull request Aug 24, 2017

test: add test-benchmark-os

dd60c60

Add minimal test to confirm that os benchmarks run. PR-URL: nodejs#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott added a commit to Trott/io.js that referenced this pull request Aug 24, 2017

test: add test-benchmark-path

ce69d01

Add minimal test to confirm that path benchmarks run. PR-URL: nodejs#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott added a commit to Trott/io.js that referenced this pull request Aug 24, 2017

test: add test-benchmark-process

3cf27f1

Add minimal test to confirm that process benchmarks run. PR-URL: nodejs#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott closed this Aug 24, 2017

maclover7 mentioned this pull request Aug 24, 2017

test, benchmark: create shared runBenchmark func #15004

Closed

6 tasks

addaleax pushed a commit to addaleax/ayo that referenced this pull request Aug 25, 2017

test: add test-benchmark-os

bac4d12

Add minimal test to confirm that os benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit to ayojs/ayo that referenced this pull request Aug 28, 2017

test: add test-benchmark-os

023da7d

Add minimal test to confirm that os benchmarks run. PR-URL: nodejs/node#14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos mentioned this pull request Sep 3, 2017

test the benchmarks at one iteration #12068

Closed

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017

test: add test-benchmark-domain

09fadc5

Add minimal test to confirm that domain benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017

test: add test-benchmark-events

566c98f

Add minimal test to confirm that events benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017

test: add test-benchmark-os

b81da62

Add minimal test to confirm that os benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017

test: add test-benchmark-path

ff3793c

Add minimal test to confirm that path benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017

test: add test-benchmark-process

25d56ad

Add minimal test to confirm that process benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins pushed a commit that referenced this pull request Sep 12, 2017

test: add test-benchmark-domain

981ef46

Add minimal test to confirm that domain benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Sep 12, 2017

test: add test-benchmark-events

7e541d6

Add minimal test to confirm that events benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Sep 12, 2017

test: add test-benchmark-os

2e3e136

Add minimal test to confirm that os benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Sep 12, 2017

test: add test-benchmark-path

b480b20

Add minimal test to confirm that path benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Sep 12, 2017

test: add test-benchmark-process

1849c51

Add minimal test to confirm that process benchmarks run. PR-URL: #14951 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added the lts-watch-v6.x label Sep 20, 2017

Trott added dont-land-on-v4.x and removed lts-watch-v6.x labels Sep 25, 2017

Trott deleted the test-benchmark-domain branch January 13, 2022 22:46

Uh oh!

Conversation

Trott commented Aug 20, 2017

Checklist

Affected core subsystem(s)

Uh oh!

targos Aug 20, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

refack Aug 20, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

refack Aug 20, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

maclover7 commented Aug 24, 2017

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

maclover7 commented Aug 24, 2017

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

MylesBorins commented Sep 20, 2017

Uh oh!

Trott commented Sep 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants