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.

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

Replace tape with tap #1171

Closed
wants to merge 1 commit into from
Closed

Conversation

NatalieWolfe
Copy link
Contributor

The tap module is a more fully featured TAP framework that executes its tests in separate child processes.

Resolves #1123

@bnoordhuis
Copy link
Member

Thanks, this is great! I wonder though, does tap.test() actually run tests in a separate process? I glean from node-tap's source that only tap.spawn() does that.

@NatalieWolfe
Copy link
Contributor Author

Correct, tap.test doesn't run each in their own process, but $ tap test/test-* will spawn a separate child process for each file that matched. Was it per-individual-test that needed process isolation?

@bnoordhuis
Copy link
Member

Yes.

@NatalieWolfe
Copy link
Contributor Author

Is it possible to move the tests which need isolation into individual files? Which tests need this? I can look into doing that step as part of this ticket.

@bnoordhuis
Copy link
Member

I'm going to say 'all of them' - it's not really true but isolation never hurts. It's my original plan of attack for #1123.

@NatalieWolfe
Copy link
Contributor Author

Is that necessary up front? That seems like the kind of thing that could be resolved on a case-by-case basis when colliding tests are discovered. Moving individual tests into separate files isn't that hard, but seems like overkill if you don't actually need them isolated.

@bnoordhuis
Copy link
Member

Such things tend to go unnoticed, though. I raised #1123 after I found two unrelated tests had been clobbering each other undetected for some time.

Another anecdote: a project I was involved in had a bug caused by lack of isolation that stopped half the tests from running but it went undiscovered for a year because the CI was green all the time.

},
"scripts": {
"test": "tape test/test-*"
"test": "tap test/test-*"
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO need to add -Rtap

@refack
Copy link
Contributor

refack commented May 15, 2017

@NatalieWolfe do you need help? I can help...

@maclover7
Copy link
Contributor

ping @NatalieWolfe

@rvagg
Copy link
Member

rvagg commented Jun 20, 2019

needs to be updated, either @NatalieWolfe needs to revisit or someone else should open a PR, I'd +1 on merging this if it was ready

@cclauss
Copy link
Contributor

cclauss commented Jul 12, 2019

@NatalieWolfe Is this already done elsewhere or is there more to do here?

@richardlau
Copy link
Member

This was done in #1795.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop using tape for tests
7 participants