Skip to content

doc: add note about autocrlf required for tests - #20752

Closed
bzoz wants to merge 2 commits into
nodejs:masterfrom
JaneaSystems:bartek-autocrlf-doc
Closed

doc: add note about autocrlf required for tests#20752
bzoz wants to merge 2 commits into
nodejs:masterfrom
JaneaSystems:bartek-autocrlf-doc

Conversation

@bzoz

@bzozbzoz commented May 15, 2018

Copy link
Copy Markdown
Contributor

Adds a note to test/README.md that setting autocrlf to true when checking out sources is required for the tests to run successfully.

See #18967, which will fail if the autocrlf is set to false.

Checklist

Adds a note to test/README.md that setting autocrlf to true when
checking out sources is required for the tests to run successfully.
Ref: nodejs#18967
@bzoz
bzoz requested a review from a team as a code ownerMay 15, 2018 18:52
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 15, 2018
@vsemozhetbytvsemozhetbyt added the windows Issues and PRs related to the Windows platform. label May 15, 2018
@vsemozhetbyt

Copy link
Copy Markdown
Contributor

@vsemozhetbytvsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 15, 2018
Comment threadtest/README.md Outdated
On how to run tests in this directory, see
[the contributing guide](../doc/guides/contributing/pull-requests.md#step-6-test).

For the test to successfully run on Windows, Node.js has to be checked out from

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: test -> tests?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Right, I'll fix it

@bzoz

bzoz commented May 16, 2018

Copy link
Copy Markdown
ContributorAuthor

@BridgeAR

Copy link
Copy Markdown
Member

This should be fixed by #20754 and adding the comment is not necessary anymore with that PR.

@BridgeARBridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 18, 2018
@BridgeAR

Copy link
Copy Markdown
Member

The issue that the comment mentions got fixed with #20754. Closing.

@bzozbzoz reopened this May 21, 2018
@bzoz

bzoz commented May 21, 2018

Copy link
Copy Markdown
ContributorAuthor

CI has autocrlf set to true, so we will never know if a change does not break test for setups with autocrlf set to false. I would add this note to the docs.

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 21, 2018

@apapirovskiapapirovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm in favour of documenting this even if it's currently not needed.

@apapirovski

Copy link
Copy Markdown
Contributor

Landed in 3654cd4

apapirovski pushed a commit that referenced this pull request May 22, 2018
Adds a note to test/README.md that setting autocrlf to true when
checking out sources is required for the tests to run successfully.
PR-URL: #20752
Ref: #18967
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
Adds a note to test/README.md that setting autocrlf to true when
checking out sources is required for the tests to run successfully.
PR-URL: #20752
Ref: #18967
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@addaleaxaddaleax mentioned this pull request May 22, 2018
MylesBorins pushed a commit that referenced this pull request May 23, 2018
Adds a note to test/README.md that setting autocrlf to true when
checking out sources is required for the tests to run successfully.
PR-URL: #20752
Ref: #18967
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.testIssues and PRs related to the tests.windowsIssues and PRs related to the Windows platform.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@bzoz@vsemozhetbyt@BridgeAR@apapirovski@Trott@cjihrig@joyeecheung@trivikr@nodejs-github-bot