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

different results than tape itself #3

Open
ljharb opened this issue Jul 21, 2020 · 13 comments · Fixed by #4
Open

different results than tape itself #3

ljharb opened this issue Jul 21, 2020 · 13 comments · Fixed by #4

Comments

@ljharb
Copy link
Contributor

ljharb commented Jul 21, 2020

I've started a branch to try out using this package here.

However, the tests fail when they should pass: https://travis-ci.com/github/ljharb/es-abstract/builds/176444693

Oddly enough, they pass in node < 8. Any ideas?

@Raynos
Copy link
Owner

Raynos commented Jul 21, 2020

Maybe i did not implement throws properly ? it proxies to node core throws()

@ljharb
Copy link
Contributor Author

ljharb commented Jul 21, 2020

it doesn’t call into tape’s “throws”? altho they should be the same I’m sure there’s differences.

@Raynos
Copy link
Owner

Raynos commented Jul 21, 2020

it uses assert.throws to check whether it should report a failure to tape.throws ;

@Raynos
Copy link
Owner

Raynos commented Jul 21, 2020

This avoids a bunch of ok foobar lines in TAP output by not calling into tape for assertions that passed.

@ljharb
Copy link
Contributor Author

ljharb commented Jul 21, 2020

that seems a bit more complex than var threw = false; try { maybeThrows(); } catch (e) { threw = true; }; would that work?

@Raynos
Copy link
Owner

Raynos commented Jul 21, 2020

Yeah but not with the regex parameter.

ljharb added a commit to ljharb/collapsed-assert that referenced this issue Jul 27, 2021
I suspect node's implementation cares about argument length.

Fixes Raynos#3 (verified locally in the project where I originally found the problem)
@ljharb
Copy link
Contributor Author

ljharb commented Jul 27, 2021

Although #4 fixes it for throws/doesNotThrow, I'm seeing a similar issue with t.equal, since node's assert.equal changed from non-strict by default to strict by default at some point. Also tape 4 uses deep-equal v1 which defaults to loose, and tape 5 uses deep-equal v2 which defaults to strict.

It seems like it'd be better to use tape's API rather than node's assert. is that something you're interested in?

@Raynos Raynos closed this as completed in #4 Jul 28, 2021
@Raynos Raynos reopened this Jul 28, 2021
@Raynos
Copy link
Owner

Raynos commented Jul 28, 2021

It would be good to use tape's API but I dont know what public or private API there available to use.

We want to run the assertions but not report them unless they fail in which case we report all of them.

I'd rather not add a direct dependency on tape though as currently the library has zero dependencies.

@ljharb
Copy link
Contributor Author

ljharb commented Jul 28, 2021

Without a direct dependency, I'm not sure how it could be injected in a way that would work for tape, and tap, and "insert tap-producer here". If it's meant to be tied to tape's API, then it should have a dependency on tape.

@Raynos
Copy link
Owner

Raynos commented Jul 28, 2021

We take t as an argument, the current design is meant to mirror assert in node core.

If assert made a breaking change i didnt pay attention to then this module should make the same breaking change :D

@ljharb
Copy link
Contributor Author

ljharb commented Jul 28, 2021

It's made a number.

If it's meant to mirror assert, then that's unfortunately much less useful, since assert throws and doesn't produce TAP.

@Raynos
Copy link
Owner

Raynos commented Jul 29, 2021

I didnt mean it like that, it should implement assert like methods like tap/tape do.

We can update the equal method to loose or not loose deep equal, either is fine.

@ljharb
Copy link
Contributor Author

ljharb commented Jul 29, 2021

Right, but the way tap and tape do that varies by version, hence the need for a peer dep.

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 a pull request may close this issue.

2 participants