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

Fail a test if its callback returns a promise that rejects #441

Merged
merged 1 commit into from
Dec 22, 2019
Merged

Fail a test if its callback returns a promise that rejects #441

merged 1 commit into from
Dec 22, 2019

Conversation

KayleePop
Copy link
Contributor

This is mentioned here #208 (comment)

The benefit is that you can use async functions as the test callback without unexpected results like hanging with no output in the browser.

If the global variable Promise isn't defined, then nothing is changed.

@KayleePop
Copy link
Contributor Author

KayleePop commented Aug 27, 2018

The build is failing because the stack in the test is slightly different in the different versions of node. It still runs correctly, but the assertion fails because it's not exactly the same TAP output.

Not sure how to deal with that.

EDIT: fixed this with a regex hack. Not sure if there is a better way.

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.

Thanks for the PR! Just highlighting this from the linked comment:

However, adding such complexity to a simple library is not something that should be done lightly or quickly.

@@ -0,0 +1,12 @@
var test = require('../../');

test('promise', function (t) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this test should be skipped when typeof Promise === 'undefined'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I skipped them using node-tap in promise_fail.js, here https://github.com/substack/tape/pull/441/files#diff-0e44c4bcf1645d05ba9b5c2e44250c26R8

If that variable skip is truthy, then these test files aren't even run. (the builds that don't support promises show the skip in the output)

Should I switch to using test.skip in tape?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm - these tests need to continue to use tap, but it'd be ideal for the skip logic to be in each test file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would produce a different (non-failing) TAP output then, which would fail the tap test instead of skipping it.

Should I put it in both? Maybe a comment explaining that they'll be skipped?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah yes, i like putting something in both - a generic "skip" mechanism in the tap test, and a specific one to promises here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a commit for this. It looks at the beginning of the stdout from the test, and if it's "skip", then it passes the test. Each test logs "skip" if Promise is undefined.

Is that what you had in mind?

@@ -0,0 +1,13 @@
var test = require('../../');

test('promise', function (t) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

also this

lib/test.js Outdated
this.emit('run');

if (typeof Promise !== 'undefined') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's ensure Promise is a function, as is Promise.resolve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Also changed in lib/test.js

var rowsString = rows.toString('utf8');

// if the output starts with skip, then skip this test
if (/^skip/.test(rowsString)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be /^skip$/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a commit that only runs the test if promises are supported.

Before it would still run the test and print the TAP output after 'skip'

@ljharb
Copy link
Collaborator

ljharb commented Dec 20, 2019

@KayleePop this needs to be rebased now that #472 has landed. I'm hoping the tests can be preserved :-)

@KayleePop
Copy link
Contributor Author

KayleePop commented Dec 21, 2019

Rebased!

I just left master's implementation alone and only rebased the tests in. The only differences were that this branch checked that Promise.resolve was a function too instead of just Promise, and this branch printed the rejection errors a little differently

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.

Thanks, this is excellent. Sorry for the long delay :-)

@ljharb ljharb added the tests label Dec 21, 2019
@ljharb ljharb merged commit ad75f86 into tape-testing:master Dec 22, 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 ^
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants