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

Outputs tests with no callbacks as a TODO test. #69

Merged
merged 1 commit into from
Dec 27, 2019

Conversation

aychtang
Copy link
Contributor

Patching #62, referencing any defined tests with no callback in the TAP output as a TODO test, similar to if the TODO directive described in TAP spec is used (https://metacpan.org/pod/release/PETDANCE/Test-Harness-2.64/lib/Test/Harness/TAP.pod#TODO-tests).

@scottcorgan
Copy link

Please!

@ghost
Copy link

ghost commented Mar 18, 2014

Should todos be counted as failures?

@scottcorgan
Copy link

Maybe as warnings/infos

@Raynos
Copy link
Collaborator

Raynos commented Mar 18, 2014

the TAP standard seems to imply that todos are failures.

I'm not sure whether empty test blocks should be failing TODOs or whether empty test blocks should count as skipped.

@aychtang
Copy link
Contributor Author

I was also on the fence about that and initially I didn't want to have them be counted as failures. However having them count as skipped didn't really make sense as that implies there was a test to skip in the first place.

The TAP spec does clearly state todos are not expected to succeed. After getting over the emotional aspect (eg: "What do you mean it's failing? I've just not implemented it yet!"), I think it does make sense, after all if you haven't yet implemented the test and feature of the spec, are you not failing in a way?

Users who don't like that a todo causes the build to fail can easily revert to the previous silent behaviour by commenting the tests that are yet to be implemented.

@Raynos
Copy link
Collaborator

Raynos commented Mar 18, 2014

This will require a major version bump since we shouldnt create failing builds for existing things that work.

@ttrfstud
Copy link

But maybe the TODO tests should have a callback, it is just that they are supposed to fail. I mean maybe these tests mean bug exists, dev is aware of that bug (the only difference from normal failing test) but has not yet fixed it yet. The note in spec about that when TODO test suddenly started passing (probably as side-effect from other changes) should add some "bonus" also seems to imply that these tests should have callback

@aychtang
Copy link
Contributor Author

aychtang commented Nov 3, 2015

Noticed this was still open, not sure who to check with on this. @Raynos?

Tests without a cb still are still silent today. If you want it implemented as a TODO / SKIP directive let me know and I can fix the conflict. Otherwise you can close the PR.

@ronkorving
Copy link
Contributor

FWIW, I've implemented TODO as a proper option to run a test, but allow it to fail (as the TAP spec indicates) in #254

@ljharb

This comment has been minimized.

@aychtang

This comment has been minimized.

@ljharb ljharb removed the needs "allow edits" PR needs "allow edits" checked label Jan 28, 2019
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Added a test, and made sure not to fail a no-callback test that's already explicitly marked as todo.

@ljharb ljharb merged commit 03529a9 into tape-testing:master Dec 27, 2019
ljharb added a commit that referenced this pull request Jan 1, 2020
Changes since v5.0.0-next.0:

 - [Breaking] fail any assertion after `.end()` is called (#489(
 - [Breaking] tests with no callback are failed TODO tests (#69)
 - [Breaking] equality functions: throw when < 2 arguments are provided
 - [Breaking] add "exports" to restrict public API
 - [Breaking] `throws`: bring into line with node’s `assert.throws`
 - [Breaking] use default `require.extensions` collection instead of the magic Array `['.js']` (#396)
 - [Fix] error stack file path can contain parens/spaces
 - [Refactor] make everything strict mode
 - [Refactor] Avoid setting message property on primitives; use strict mode to catch this (#490)
 - [Refactor] generalize error message from calling `.end` more than once
 - [Dev Deps] update `eslint`
 - [Tests] improve some failure output by adding messages
 - [Tests] handle stack trace variation in node <= 0.8
 - [Tests] ensure bin/tape is linted
 - [Tests] Fail a test if its callback returns a promise that rejects (#441)
 - [eslint] fix remaining undeclared variables (#488)
 - [eslint] Fix leaking variable in tests
 - [eslint] fix object key spacing

Changes since v4.12.1:

 - [Breaking] `error` should not emit `expected`/`actual` diags (#455)
 - [Breaking] support passing in an async function for the test callback (#472)
 - [Breaking] update `deep-equal` to v2
 - [Deps] update `resolve`
 - [meta] change dep semver prefix from ~ to ^
ljharb added a commit that referenced this pull request Jan 19, 2020
Changes since v5.0.0-next.3:
 - [Fix] `.catch` is a syntax error in older browsers
 - [Refactor] remove unused code
 - [Deps] update `resolve`

Changes since v4.13.0:

 - [Breaking] update `deep-equal` to v2
 - [Breaking] fail any assertion after `.end()` is called (#489)
 - [Breaking] `error` should not emit `expected`/`actual` diags (#455)
 - [Breaking] support passing in an async function for the test callback (#472)
 - [Breaking] tests with no callback are failed TODO tests (#69)
 - [Breaking] equality functions: throw when < 2 arguments are provided
 - [Breaking] add "exports" to restrict public API
 - [Breaking] `throws`: bring into line with node’s `assert.throws`
 - [Breaking] use default `require.extensions` collection instead of the magic Array `['.js']` (#396)
 - [meta] change dep semver prefix from ~ to ^
ljharb added a commit that referenced this pull request Mar 3, 2020
Changes since v5.0.0-next.4:
 - [Breaking] only `looseEqual`/`deepEqual, and their inverses, are now non-strict.
 - [Breaking] make equality functions consistent:
 - [Breaking] `equal`: use `==`, not `===`, to match `assert.equal`
 - [Breaking] `strictEqual`: bring `-0`/`0`, and `NaN` into line with `assert`
 - [patch] Print name of test that didnt end (#498)
 - [Refactor] remove unused code
 - [Deps] update `resolve`

Changes since v4.13.2:

 - [Breaking] only `looseEqual`/`deepEqual, and their inverses, are now non-strict.
 - [Breaking] make equality functions consistent:
 - [Breaking] `equal`: use `==`, not `===`, to match `assert.equal`
 - [Breaking] `strictEqual`: bring `-0`/`0`, and `NaN` into line with `assert`
 - [Breaking] update `deep-equal` to v2
 - [Breaking] fail any assertion after `.end()` is called (#489)
 - [Breaking] `error` should not emit `expected`/`actual` diags (#455)
 - [Breaking] support passing in an async function for the test callback (#472)
 - [Breaking] tests with no callback are failed TODO tests (#69)
 - [Breaking] equality functions: throw when < 2 arguments are provided
 - [Breaking] add "exports" to restrict public API
 - [Breaking] `throws`: bring into line with node’s `assert.throws`
 - [Breaking] use default `require.extensions` collection instead of the magic Array `['.js']` (#396)
 - [meta] change dep semver prefix from ~ to ^
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major: breaking change Any breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants