Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: add test-benchmark-assert #15174

Closed
wants to merge 3 commits into from
Closed

test: add test-benchmark-assert #15174

wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 4, 2017

First commit:

benchmark: enable assert benchmark with short len

`deepequal-typedarrays.js` throws if `len` is set to 100 or less due to
a hardcoded index. Calculate the index based on `len` so benchmark can
be run with small `len` values if desired.

Second commit:

benchmark: provide default methods for assert

The benchmarks for `assert` all take a `method` configuration option,
but the allowable values are different across the files. For each
benchmark, provide an arbitrary default if `method` is set to an empty
string. This allows all the `assert` benchmarks to be run with a single
command but only on a single method. This is primarily useful for
testing that the assert benchmark files don't contain egregious errors.
(In other words, it's useful for testing.)

Third commit:

test: add test-benchmark-assert

Add minimal test for `assert` benchmarks.
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 assert

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. benchmark Issues and PRs related to the benchmark subsystem. test Issues and PRs related to the tests. labels Sep 4, 2017
targos
targos reviewed Sep 4, 2017
View reviewed changes
benchmark/assert/deepequal-typedarrays.js Outdated
@@ -33,7 +33,8 @@ function main(conf) {
const actual = new clazz(len);
const expected = new clazz(len);
const expectedWrong = Buffer.alloc(len);
expectedWrong[100] = 123;
const wrongIndex = Math.floor(n / 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you intend to use len?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you intend to use len?

@targos Yes, oops!

BridgeAR
BridgeAR approved these changes Sep 7, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % comments.

benchmark/assert/deepequal-buffer.js
@@ -27,6 +27,7 @@ function main(conf) {
data.copy(expectedWrong);

switch (conf.method) {
case '':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be so kind and a a comment why this is necessary? Otherwise I would be confused by this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done here and in the other 8 places it occurs in other files.

@BridgeAR
Copy link
Member

Ping @Trott

@Trott
benchmark: enable assert benchmark with short len
5a2ce48 
`deepequal-typedarrays.js` throws if `len` is set to 100 or less due to
a hardcoded index. Calculate the index based on `len` so benchmark can
be run with small `len` values if desired.
@Trott
benchmark: provide default methods for assert
9e3098c 
The benchmarks for `assert` all take a `method` configuration option,
but the allowable values are different across the files. For each
benchmark, provide an arbitrary default if `method` is set to an empty
string. This allows all the `assert` benchmarks to be run with a single
command but only on a single method. This is primarily useful for
testing that the assert benchmark files don't contain egregious errors.
(In other words, it's useful for testing.)
@Trott
test: add test-benchmark-assert
2405a3f 
Add minimal test for `assert` benchmarks.
@Trott
Copy link
Member Author

Trott commented Sep 13, 2017 •
edited
Loading

I've refactored the test to use the common/benchmark.js module that was added by @maclover7 recently.

@Trott
Copy link
Member Author

Trott commented Sep 13, 2017

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

@jasnell
Copy link
Member

jasnell commented Sep 15, 2017

Landed in 11f46a2, 3c4c0db, and a901849

@jasnell jasnell closed this Sep 15, 2017
@jasnell
Copy link
Member

jasnell commented Sep 20, 2017

This lands cleanly in v8.x-staging but the test fails because ../common/benchmark does not exist. It needs a backport PR. Ping @Trott

@jasnell jasnell mentioned this pull request Sep 20, 2017
Merged
@Trott
Copy link
Member Author

Trott commented Sep 25, 2017

@jasnell It looks like you subsequently landed these, obviating the need for a backport. I'm going to remove the backport-requested label. Let me know if that's wrong and/or you need anything more from me on this.

@MylesBorins
Copy link
Contributor

lts?

@Trott
Copy link
Member Author

Trott commented Oct 18, 2017

lts?

If it lands cleanly, yes. If it doesn't, probably not worth it.

@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

@Trott
Copy link
Member Author

Trott commented Oct 21, 2017

@MylesBorins Is this supposed to be labeled do-not-land-on-v6.x rather than land-on-v6.x?

@MylesBorins
Copy link
Contributor

@Trott indeed

@Trott Trott deleted the vacay-9 branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem. benchmark Issues and PRs related to the benchmark subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Trott @BridgeAR @jasnell @MylesBorins @targos @nodejs-github-bot