Skip to content

[backport to v6] test: begin normalizing fixtures use #16265

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
Closed
wants to merge 34 commits into from
Closed

[backport to v6] test: begin normalizing fixtures use #16265

wants to merge 34 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 17, 2017 •
edited
Loading

Original PR: #14332

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test

danbev and others added 30 commits October 17, 2017 16:05
@danbev @MylesBorins
build: call setlocal in vcbuild.bat
b8ab430 
Currently the variables set in vcbuild.bat are mostly global and
escape/leak out into the calling process. For example, running
vcbuild.bat test and then echoing the config variable gives:

vcbuild.bat test
...
echo %config%
Release

After this change the same command give:
vcbuild.bat test
...
echo %config%
%config%

PR-URL: nodejs#15754
Reviewed-By: James M Snell <jasnell@gmail.com>
@tshemsedinov @MylesBorins
doc: fix incorrect vm.createContext usage
3991953 
In code example `vm.createContext` called with new operator by mistake.
It is not a constructor.

PR-URL: nodejs#16059
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@TimothyGu @MylesBorins
src: fix ^ in stack trace with vm's columnOffset
c405f95 
While VM module's columnOffset option does succeed in applying an offset
to the column number in the stack trace, the wavy diagram printed does
not account for potential offsets, resulting in erroneous location of
`^` in the first line of the script.

Before:

```
> vm.runInThisContext('throw new Error()', { columnOffset: 5 })
evalmachine.<anonymous>:1
throw new Error()
     ^

Error
    at evalmachine.<anonymous>:1:12
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at Object.runInThisContext (vm.js:116:38)
```

After:

```
> vm.runInThisContext('throw new Error()', { columnOffset: 5 })
evalmachine.<anonymous>:1
throw new Error()
^

Error
    at evalmachine.<anonymous>:1:12
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at Object.runInThisContext (vm.js:139:38)
    at repl:1:4
```

PR-URL: nodejs#15771
Refs: jsdom/jsdom#2003
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@Saeed-Navarik @MylesBorins
test: remove messages in assert.strictEqual
c81b425 
PR-URL: nodejs#16014
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@Trott @MylesBorins
doc: update style guide for markdown extension
6d49154 
There are no longer files in the repository that use the `.markdown`
extension so remove mention of them.

PR-URL: nodejs#15786
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@gishmel @MylesBorins
test: alter assert.strictEqual to default message
d0157b7 
PR-URL: nodejs#15978
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins
test: replacing assert message with template
0c0717c 
PR-URL: nodejs#15974
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@ericljpemberton @MylesBorins
test: improve assert messages
bf1bf28 
PR-URL: nodejs#15972
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@craftninja @MylesBorins
doc: add clearer setup description
d3544c1 
Updated console example to follow style of rest of the examples

PR-URL: nodejs#15962
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@brantphoto @MylesBorins
test: replace literal with template string
b43fed4 
PR-URL: nodejs#15957
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@sarahmeyer @MylesBorins
tools: use template literals
97cf4f1 
PR-URL: nodejs#15956
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@dpaulino @MylesBorins
test: removed string from assert message arg
a686f9c 
PR-URL: nodejs#15954
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@joanne-jjb @MylesBorins
test: changes to use template literal
b93842a 
PR-URL: nodejs#15937
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@fyesoft @MylesBorins
test: remove literal error messages
3371ee8 
Assertions will now print the values that caused the assertions
to fail.

PR-URL: nodejs#15928
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@garlicbrie @MylesBorins
test: changed buffer-zero output
c53ed73 
Added interpolated strings to display the error value

PR-URL: nodejs#15926
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@SgtPooki @MylesBorins
test: display better error message for assertion
ad25c28 
This commit makes understanding assertion failures easier by
displaying the values that failed the assertion.

PR-URL: nodejs#15883
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@domrein @MylesBorins
test: replaced literals in errors with templates
44f3efc 
PR-URL: nodejs#15911
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@Suraiya-Hameed @MylesBorins
doc: fix macosx-firewall suggestion BUILDING
d179660 
Adding back the changes made by commit# 791d560, that suggests running
macosx-firewall.sh script after bulid step. These changes were deleted
by commit# fc102d0, but they are still applicable.

PR-URL: nodejs#15829
Reviewed-By: Ryan Graham <r.m.graham@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@pomerantsev @MylesBorins
test: improve an error message
46f4597 
PR-URL: nodejs#15921
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@justin0022 @MylesBorins
test: remove message from asserts
cd8d15e 
PR-URL: nodejs#15920
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@genewoo @MylesBorins
test: improve asset msg in test
f469169 
Add result to part of assert message.

PR-URL: nodejs#15918
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@robtpaton @MylesBorins
test: replace string concatenation with template
06519b8 
PR-URL: nodejs#15915
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@danielelisi @MylesBorins
test: add NODE_UNIQUE_ID value to err message
79667a8 
PR-URL: nodejs#15914
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@sashimii @MylesBorins
test: replace error msg w/ template literal
734642d 
PR-URL: nodejs#15910
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@jmcgui05 @MylesBorins
test: assert.strictEqual using template literals
51db86d 
PR-URL: nodejs#15944
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@craftninja @MylesBorins
test: updated error message
c00f719 
PR-URL: nodejs#15906
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@RafLeszczynski @MylesBorins
test: cleanup test-buffer-sharedarraybuffer
88e6ecc 
remove third argument `0` from calls to `assert.deepStrictEqual`

PR-URL: nodejs#15896
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@keywordnew @MylesBorins
test: include expected result in error messages
ac7db84 
PR-URL: nodejs#16039
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
tools: replace concat with template literals
f54608e 
PR-URL: nodejs#16046
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Lance Ball <lball@redhat.com>
@joyeecheung @MylesBorins
doc: mention collaboration summit in onboarding.md
f19096f 
PR-URL: nodejs#16079
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
kfarnung and others added 4 commits October 17, 2017 16:45
@kfarnung @MylesBorins
doc: add kfarnung to collaborators
26b8c8d 
PR-URL: nodejs#16108
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@cpandrews8 @MylesBorins
test: clarify assert messages in crypto tests
3e5e8cb 
PR-URL: nodejs#16019
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Lance Ball <lball@redhat.com>
@johenry @MylesBorins
test: remove assert message
3996e8a 
PR-URL: nodejs#15997
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell @refack
test: begin normalizing fixtures use
a721d23 
Adds a new `../common/fixtures' module to begin normalizing
`test/fixtures` use. Our test code is a bit inconsistent with
regards to use of the fixtures directory. Some code uses
`path.join()`, some code uses string concats, some other
code uses template strings, etc. In mnay cases, significant
duplication of code is seen when accessing fixture files, etc.

This updates many (but by no means all) of the tests in the
test suite to use the new consistent API. There are still
many more to update, which would make an excelent Code-n-Learn
exercise.

PR-URL: nodejs#14332
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@refack refack added the v6.x label Oct 17, 2017
@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. v6.x labels Oct 17, 2017
@refack refack mentioned this pull request Oct 17, 2017
Closed
4 tasks
jasnell
jasnell approved these changes Oct 18, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM

MylesBorins pushed a commit that referenced this pull request Oct 19, 2017
@jasnell @MylesBorins
test: begin normalizing fixtures use
db37c88 
Adds a new `../common/fixtures' module to begin normalizing
`test/fixtures` use. Our test code is a bit inconsistent with
regards to use of the fixtures directory. Some code uses
`path.join()`, some code uses string concats, some other
code uses template strings, etc. In mnay cases, significant
duplication of code is seen when accessing fixture files, etc.

This updates many (but by no means all) of the tests in the
test suite to use the new consistent API. There are still
many more to update, which would make an excelent Code-n-Learn
exercise.

Backport-PR-URL: #16265
PR-URL: #14332
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins
Copy link
Contributor

Thanks @refack

landed in db37c88

@refack refack deleted the backport-14332-to-v6 branch October 19, 2017 19:49
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@jasnell @MylesBorins
test: begin normalizing fixtures use
a3f3d99 
Adds a new `../common/fixtures' module to begin normalizing
`test/fixtures` use. Our test code is a bit inconsistent with
regards to use of the fixtures directory. Some code uses
`path.join()`, some code uses string concats, some other
code uses template strings, etc. In mnay cases, significant
duplication of code is seen when accessing fixture files, etc.

This updates many (but by no means all) of the tests in the
test suite to use the new consistent API. There are still
many more to update, which would make an excelent Code-n-Learn
exercise.

Backport-PR-URL: #16265
PR-URL: #14332
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2017
@jasnell @MylesBorins
test: begin normalizing fixtures use
1fdbaed 
Adds a new `../common/fixtures' module to begin normalizing
`test/fixtures` use. Our test code is a bit inconsistent with
regards to use of the fixtures directory. Some code uses
`path.join()`, some code uses string concats, some other
code uses template strings, etc. In mnay cases, significant
duplication of code is seen when accessing fixture files, etc.

This updates many (but by no means all) of the tests in the
test suite to use the new consistent API. There are still
many more to update, which would make an excelent Code-n-Learn
exercise.

Backport-PR-URL: #16265
PR-URL: #14332
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

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

Successfully merging this pull request may close these issues.