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

build: add a testclean target #29094

Closed
wants to merge 2 commits into from
Closed

build: add a testclean target #29094

wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Aug 12, 2019

This commit adds a target named testclean to allow for cleaning the
temporary files generated during a test run without having to use the clean target.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@danbev
build: add a testclean target
a3f291d 
This commit adds a target named testclean to allow for cleaning the
temporary files generated during a test run without having to use the
clean target.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 12, 2019
Trott
Trott reviewed Aug 12, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Can you add this to vcbuild.bat too?

@danbev
Copy link
Contributor Author

danbev commented Aug 14, 2019

Can you add this to vcbuild.bat too?

I'll take a look at adding this to vcbuild.bat today. Thanks

@Trott
Copy link
Member

Trott commented Aug 14, 2019

Can you add this to vcbuild.bat too?

I'll take a look at adding this to vcbuild.bat today. Thanks

Looks like vcbuild.bat never cleans tmp directories, even if vcbuild.bat clean is called, so this would be adding new additional functionality to vcbuild.bat beyond what is being added to Makefile here. I'm not even sure that we put the tmp directories in the same place on Windows as we do on POSIX. I'm OK leaving this off. Someone can implement it later if it's desirable to have on Windows too.

@Trott
Copy link
Member

Trott commented Aug 14, 2019

I'm not even sure that we put the tmp directories in the same place on Windows as we do on POSIX.

(We do.)

@danbev
Copy link
Contributor Author

danbev commented Aug 16, 2019

@Trott I've added a testclean target which can be called using:

danbev@win2 MINGW64 ~/node (master)
$ ./vcbuild.bat testclean
deleting test/.tmp*

Sorry this took so long, I had to set up a windows vm on azure as I had deleted my local window vm due to resource constraints.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

Trott pushed a commit that referenced this pull request Aug 16, 2019
@danbev @Trott
build: add a testclean target
a890771 
This commit adds a target named testclean to allow for cleaning the
temporary files generated during a test run without having to use the
clean target.

PR-URL: #29094
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Aug 16, 2019

Landed in a890771

@Trott Trott closed this Aug 16, 2019
targos pushed a commit that referenced this pull request Aug 19, 2019
@danbev @targos
build: add a testclean target
e51f924 
This commit adds a target named testclean to allow for cleaning the
temporary files generated during a test run without having to use the
clean target.

PR-URL: #29094
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request Aug 19, 2019
Merged
@danbev danbev deleted the build-add-testclean-target branch August 20, 2019 07:36
This was referenced Aug 20, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@danbev @Trott @nodejs-github-bot @jasnell @addaleax