Skip to content

Feature Request: A way to mark a test 'skipped' after it starts #2010

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

Open
coreyfarrell opened this issue Jan 9, 2019 · 12 comments
Open

Feature Request: A way to mark a test 'skipped' after it starts #2010

coreyfarrell opened this issue Jan 9, 2019 · 12 comments

Comments

@coreyfarrell
Copy link
Contributor

coreyfarrell commented Jan 9, 2019

Description

I'm using ava to control selenium-webdriver for tests run in Firefox and Chrome. It cannot be known synchronously if the browser can be run. An example flow could be:

test('somepage.html', async t => {
  try {
    await loadPage('somepage.html');
  } catch (error) {
    t.skip();
    return;
  }
  // testing against the page here
});

In this example loadPage() will reject if the browser cannot be started, otherwise it will resolve. The goal is that the test will report skipped if the browser cannot be run.

It would probably be good if t.skip() threw an exception if any assertions have already run for that test.

Environment

Node.js v10.14.2
linux 4.19.8-200.fc28.x86_64
ava 1.0.1
npm 6.4.1
@novemberborn
Copy link
Member

That's an interesting use case. Calling it skip may be misleading, since the test did execute. We have t.pass() and t.fail(). Perhaps t.warn()?

Combined with #1692 you could even let some assertions fail, discard them, and then warn that the test could not be completed.

@sindresorhus thoughts?

@coreyfarrell
Copy link
Contributor Author

My hope is for the final report to report to state that tests for unavailable browsers to be 'skipped'. Specifically when a specific browser cannot be run it should not report pass or fail for the associated tests.

BTW I've edited my example code to show that you would return from the test after calling t.skip(). I'm open to different naming, maybe t.skipped() or t.dependencyMissing()?

@novemberborn
Copy link
Member

Yes that was my understanding. I'm proposing that t.warn() puts the test in a state where it has neither failed nor passed. It wouldn't impact the exit code. You could still return, perhaps we should make that mandatory — e.g. if there is another failing assertion in the same test that would override the warning.

@coreyfarrell
Copy link
Contributor Author

coreyfarrell commented Jan 11, 2019

Agreed that it should be mandatory to return after calling this function. One hesitation about t.warn is that you have t.log. I've always felt that t.warn should exist and be similar to console.warn the same way t.log is like console.log. Not sure if that could cause confusion?

@novemberborn
Copy link
Member

Yes I was wondering about warn… if we can do some bike shedding, the problem is that the test can't pass due to circumstances out of its control, and yet this shouldn't fail CI. Perhaps t.cancel()?


That said, is it feasible to determine whether the test can run before you declare any tests? Once you call test() (or any hooks) you must declare all tests synchronously, but that first call is allowed to be asynchronous.

@coreyfarrell
Copy link
Contributor Author

t.cancel definitely seems better than t.warn.


Very interesting, I didn't know it was possible to defer the first registration to ava. This might be a solution to my specific problem, though honestly something like t.cancel would be easier.

@coreyfarrell
Copy link
Contributor Author

I've tweaked the way my tests are declared so they do not get created until after the browser is successfully started, though this does have one drawback. An example test declaration is:

page('check-text.html', async t => {
	const {selenium, checkText} = t.context;
	const ele = await selenium.findElement({id: 'test'});

	await checkText(ele, 'This is a test!');
});

The page function adds the title and callback to an internal array, then after the browser is successfully started it performs a bunch of calls to test() or test.skip(). If this results in an exception due to duplicate test name then the backtrace is wrong, so having page directly call test() with an implementation that conditionally calls t.cancel() would be preferable for the purpose of duplicate title error reporting. This is a minor issue though.

@sindresorhus sindresorhus changed the title Feature Request: A way to mark a test 'skipped' after it starts. Feature Request: A way to mark a test 'skipped' after it starts Feb 18, 2019
@sindresorhus
Copy link
Member

sindresorhus commented Feb 18, 2019

I like the idea (and naming) of t.cancel(). I can see that being useful for many situations.

@coreyfarrell
Copy link
Contributor Author

So in my use case t.cancel() would be called before any t assertions and be followed by an immediate return:

test('test with async resolvable external dependency', async t => {
  let externalSystem;
  try {
    externalSystem = await startExternalSystem();
  } catch (error) {
    t.cancel();
    return;
  }

  // perform testing with `externalSystem`.
  t.is(typeof externalSystem, 'object');
});

So the questions I have - how should ava react if t.cancel() and t assertions are both run by the same test? In that case should order matter - t.true(true);t.cancel(); vs t.cancel();t.true(true);?

Also should the promise returned by the test matter to how t.cancel() is interpreted? In the example above the async function returns Promise.resolve(), which I think should cause the test to be reported as 'skipped'. What if t.cancel();return; were replaced with t.cancel();throw error;? Should the rejected promise be treated as an expected failure like t.failing()? Same question for a uncaught throw after t.cancel().

Sorry to bombard with questions, just trying to understand the edge cases.

@novemberborn
Copy link
Member

I'd say you can only cancel if you haven't run assertions: Running assertions on a canceled test causes the test to fail. Canceling a test that has run assertions causes the test to fail.

We'll introduce a new reporting category for canceled tests.

@tommy-mitchell
Copy link
Contributor

Now that assertions throw, would the return be necessary? I think that tests should still fail if they're cancelled after running assertions.

@novemberborn
Copy link
Member

No, t.cancel() can also throw, just like a failed assertion.

I think if an assertion has already failed, then canceling has no effect. (To even be able to cancel you'd need to have caught the failed assertion error, which you're not supposed to do.)

A failed assertion after a cancellation (e.g. async) should also still fail the test IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants