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

Breaking change in 4.10.0? #458

Closed
ralphtheninja opened this issue Feb 10, 2019 · 6 comments
Closed

Breaking change in 4.10.0? #458

ralphtheninja opened this issue Feb 10, 2019 · 6 comments

Comments

@ralphtheninja
Copy link

ralphtheninja commented Feb 10, 2019

Greenkeeper warned about 4.10.0 breaking tests and when reverting back to previous 4.9.x they started working again. For instance https://travis-ci.org/Level/codec/jobs/491075876

Seems to be related to streams somehow.

cc @vweevers

@ljharb
Copy link
Collaborator

ljharb commented Feb 10, 2019

4.10 includes some bug fixes (#403, #404) that might cause this.

Specifically, though, the tests have some issues -
https://github.com/Level/codec/blob/ab18d82d2d36446c21f0a8a3e4702d32b15e57e9/test/decoder.js#L4 does not include a plan or an end (directly on the t), and neither does
https://github.com/Level/codec/blob/ab18d82d2d36446c21f0a8a3e4702d32b15e57e9/test/decoder.js#L38. If you add the outer t.end() calls, what happens to the tests in tape 4.9?

@vweevers
Copy link
Contributor

If tape < 4.10 did not need an explicit t.end() after one or more t.test() - which is reasonable - then this is a breaking change.

@vweevers
Copy link
Contributor

vweevers commented Feb 10, 2019

That said, adding t.end() does fix the tests (works in both 4.9 and 4.10), at least for level-codec and abstract-leveldown.

@vweevers
Copy link
Contributor

After looking at #403 and its tests, I understand why t.end() or t.plan() is needed. It's to support the use case of assertions mixed with subtests:

test('example', function (t) {
  t.plan(2)

  t.ok(true)
  t.test(function (t) {
    // ..
  })
})

@ralphtheninja I'll make some PRs for our tests.

@vweevers
Copy link
Contributor

Good to close. Thanks @ljharb!

@ljharb
Copy link
Collaborator

ljharb commented Feb 10, 2019

Thanks! Unfortunately bug fixes can still be arguably breaking changes; in this case, i think it’s a bug fix, and I’m glad you were able to adjust to it easily :-)

geraintwhite added a commit to geraintwhite/async-tools that referenced this issue Feb 10, 2019
geraintwhite added a commit to geraintwhite/markdown-templator that referenced this issue Feb 10, 2019
geraintwhite added a commit to itsapi/post-receive that referenced this issue Feb 10, 2019
orangejulius added a commit to pelias/microservice-wrapper that referenced this issue Feb 11, 2019
In a recent release, `tape` fixed a bug
(tape-testing/tape#458) which ends up also being a
major breaking change.

Essentially, any time tests are nested, both the nested and top level
tests must call `end()`. This appears to have affected many of our
repositories, as we heavily use nested tests.

Fixes #33
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

3 participants