Skip to content

test: split parallel into subsystems - #6230

Closed
bengl wants to merge 1 commit into
nodejs:masterfrom
bengl:testsubdirs
Closed

bengl wants to merge 1 commit into
nodejs:masterfrom
bengl:testsubdirs

Conversation

@bengl

@bengl bengl commented Apr 15, 2016 •

Copy link
Copy Markdown
Member
Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Allow running tests in subdirectories of test/parallel. Currently it only works on test/parallel, but it could be changed to work in other directories. (Done in test/testpy/__init__.py.)

Split up most tests in test/parallel into reasonable subdirectories. There are some stragglers that didn't seem to make sense in subdirs, but I'm happy to change that if desired.

Fixes: #6217 (If it's decided that this is worth doing.)

Allow running tests in subdirectories of test/parallel.

Split up most tests in test/parallel into reasonable subdirectories.

Fixes: nodejs#6217
@mscdex mscdex added the test Issues and PRs related to Node.js core tests and test infrastructure. label Apr 15, 2016
@thefourtheye

Copy link
Copy Markdown
Contributor

Can you split the commit? Moving files to different directories as a commit and the Python changes necessary as another one

@jbergstroem

jbergstroem commented Apr 17, 2016 •

Copy link
Copy Markdown
Member

@thefourtheye perhaps hold off spending extra time until there's a consensus this will should land?

@thefourtheye

Copy link
Copy Markdown
Contributor

@jbergstroem Sure.

@bengl

bengl commented Apr 18, 2016

Copy link
Copy Markdown
Member Author

Yes, happy to split it up once it's decided that this should be done. (Also rebase, etc.)

@orangemocha

Copy link
Copy Markdown
Contributor

Reminder to self :

  • Check that the --run argument still works as intended
  • Check that status files still work as intended. Might need to update instructions for marking tests as flaky.

@imyller

imyller commented Sep 24, 2016

Copy link
Copy Markdown
Member

Looks like a good idea, but PR and discussion has stalled.

Can we reach consensus?

/ping @bengl still interested in pushing this effort forward?

@Trott

Trott commented Sep 25, 2016

Copy link
Copy Markdown
Member

I don't have a strong opinion either way on this one, but I know that there are people who still feel like they deal with fallout from the move from test/simple to test/parallel and friends, so definitely try to loop them in if this does start moving again. I don't remember who it was, but it was likely a subset of indutny, bnoordhuis, and piscisaureus.

@bnoordhuis

Copy link
Copy Markdown
Member

My beef was mostly with git log ending at commit 0e19476 all the time but that's a paper-cut issue compared to being able to run tests in parallel.

GitHub not displaying test/ correctly is a much less compelling reason to split up everything again, IMO. Report it to GH and have them fix it instead of trying to work around it.

@targos

targos commented Sep 25, 2016

Copy link
Copy Markdown
Member

My beef was mostly with git log ending at commit 0e19476 all the time

Unless you are talking about the log of directories, you can do git log --follow path/to/file to get the full log of a file.

@bnoordhuis

Copy link
Copy Markdown
Member

I know, I have log.follow set in my .gitconfig, but that only works for a single file.

@rvagg
rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
@bengl

bengl commented Oct 27, 2016

Copy link
Copy Markdown
Member Author

Gonna close this since if we do it, it should probably just be redone.

@bengl bengl closed this Oct 27, 2016
@jasnell

jasnell commented Oct 27, 2016

Copy link
Copy Markdown
Member

Fwiw, I'd still really like to see this happen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants