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

Test exited without ending #459

Closed
linuxdropout opened this issue Feb 12, 2019 · 11 comments
Closed

Test exited without ending #459

linuxdropout opened this issue Feb 12, 2019 · 11 comments

Comments

@linuxdropout
Copy link

linuxdropout commented Feb 12, 2019

All nested tests now exit without ending.

Tape version: ^4.9.1 (latest)

tape('example', ({ test }) => {
  test('nested', t => {
    t.end()
  })
})

This code will fail to run.

@Qard
Copy link

Qard commented Feb 12, 2019

Yeah, it seems t.end() is now required, even when only creating nested tests.

tape('example', t => {
  t.test('nested', tt => {
    tt.end()
  })
  t.end()
})

@ljharb
Copy link
Collaborator

ljharb commented Feb 12, 2019

An explicit t.plan or t.end has always been supposed to be required, at every level - this bug was fixed in v4.10.

@orangejulius
Copy link

@ljharb even thought this is technically a bug fix, it seems that many people rely on this broken behavior.

As someone maintaining several repositories that now have tests failing due to this change, I'd like to request reverting the bugfix in the 4.x line, and releasing a 5.x with the bug fixed.

That will allow everyone using semver to upgrade on their own schedule

@Qard
Copy link

Qard commented Feb 12, 2019

As someone working on a project with literally thousands of nested tests like this, I agree--a major release fix would be nice here...

@ljharb
Copy link
Collaborator

ljharb commented Feb 12, 2019

Yeah, this is a fair point.

Do you think that it would be a reasonable patch release to explicitly allow a test that only has nested tests to not require the outer end/plan? iow, the requirement would present itself the instant you had a top-level non-nested-test assertion.

My hope is that that way we wouldn't have to jump to v5, we'd keep the bugfix in v4.10, and we'd remove the friction you are experiencing.

@orangejulius
Copy link

Do you think that it would be a reasonable patch release to explicitly allow a test that only has nested tests to not require the outer end/plan?

That would work for our codebase as far as I know, and it sounds like a good middle ground.

Personally I would prefer keeping things simple with a revert to unblock progress (Greenkeeper is already getting confused about other package updates since our tests are failing).

That said, I appreciate the time it will take to fix this either way, so I'd be fine with either. Thanks for taking a look at it :)

@ljharb
Copy link
Collaborator

ljharb commented Feb 12, 2019

@orangejulius if I can't fix it in the next day or so, I'll plan to revert, but I always prefer rolling forward than rolling back, since you can pin to ~4.9 of tape in the meantime to unblock yourself :-)

@Qard
Copy link

Qard commented Feb 12, 2019

I believe we do not mix nested tests with other assertions, so that seems like a reasonable approach to me. I can't really speak for what the expectations are for the rest of the community though.

@linuxdropout
Copy link
Author

linuxdropout commented Feb 13, 2019

+1 on the 'test that has only nested tests and no other assertions'. Would fix our library with literally 1 million tests written like this.

@ljharb
Copy link
Collaborator

ljharb commented Feb 13, 2019

Ended up reverting; I'll reopen #222. Hopefully v4.10.1 fixes this.

ljharb added a commit that referenced this issue Feb 14, 2019
 - [Fix] Partial revert of #403: fbe4b95 and 367b010 (#459, #222)
 - [Refactor] consistent spacing
 - [Deps] update `resolve`
@orangejulius
Copy link

4.10.1 works great for us. Thanks for taking the time to handle this!

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

No branches or pull requests

4 participants