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

Planned assertion count / Hook into timeout? (for testing runtime) #44125

Closed
markwylde opened this issue Aug 3, 2022 · 18 comments
Closed

Planned assertion count / Hook into timeout? (for testing runtime) #44125

markwylde opened this issue Aug 3, 2022 · 18 comments
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.

Comments

@markwylde
Copy link

markwylde commented Aug 3, 2022

What is the problem this feature will solve?

When writing tests, I really like the plan function in tape.

In summary it lets you specify how many assertions you expect to have, then the test will fail if there are fewer/more than planned.

I'm really loving the new built in testing runtime, but I'm really missing plan. So I've built a wrapper around assert that implements the plan function in a similar way.

import assertPlan from 'assert-plan';

test('test 1 assertion', async t => {
  const assert = assertPlan(1);

  assert.strictEqual(1, 1);

  await assert.wait();
});

https://github.com/markwylde/assert-plan#example-usage

It's working really well, but there's a problem when the test timesout. I would like to log out the test timed out due to not receiving enough planned assertions.

To get around this I have to provide two timeouts, as below: (it works, but it's not very nice):

For example

Notes:

  • You can see there is a top timeout of 2000ms, then the assert will timeout at 1500ms.
  • When the assertPlan times out, it throws and error that is outputted to the user.
  • this test will hang because it's expecting 2 asserions
  • it only receives 1 assertion, therefore throws after 1500
test('test 2 assertions will timeout', { timeout: 2000 }, async t => {
  const assert = assertPlan(2, { timeout: 1500 });

  assert.strictEqual(1, 1);

  await assert.wait();
});

What is the feature you are proposing to solve the problem?

I think there's probably an obvious existing solution, or I'm trying to solve something in a strange way. So I'm really open to criticism on my approach. But some ideas off the top of my head are:

Proposal 1 - expose the timeout on t

test('test 2 assertions will timeout', { timeout: 2000 }, async t => {
  const assert = assertPlan(2, { timeout: t.timeout });
});

Proposal 2 - expose an event when a test timesout

test('test 2 assertions will timeout', { timeout: 2000 }, async t => {
  const assert = assertPlan(2);
  t.onTimeout(assert.cancel);
});

Proposal 3 - test timeout hook

import { onTimeout } from 'node:test';

// triggered anytime a test times out
onTimeout((testFunction) => {
  assertPlan.outputPendingAssertPlans();
});

test('test 2 assertions will timeout', { timeout: 2000 }, async t => {
  const assert = assertPlan(2);
  t.onTimeout(assert.cancel);
});

What alternatives have you considered?

I originally thought if we could add a plan function to the test runtime, like tape does:

For example:

import assertPlan from 'assert-plan';

test('test 2 assertions will timeout', { timeout: 2000 }, async t => {
  t.plan(2);
 assert(1 + 1, 2, '1+1=2');
});

But it breaks the cleanest of the test module, and how it's not coupled with assert at all. So I feel the right solution is to wrap/use an alternative assert library.

Maybe we could add a plan to the assert module, but you could be running multiple tests in parallel and assert wouldn't have a way of accumulating counts for just the specific test you are running.

You could make assert a constructor, but even then the problem of how to timeout the wait when the test timeouts still exists.

Thank you for humoring my idea and sorry if there is some really obvious stuff I'm missing here.

@markwylde markwylde added the feature request Issues that request new features to be added to Node.js. label Aug 3, 2022
@markwylde markwylde changed the title Planned assertion count / Hook into timeout? Planned assertion count / Hook into timeout? (for testing runtime) Aug 3, 2022
@markwylde
Copy link
Author

Just to add a more real world example if it helps.

In the below example, the test will wait if chatbot never emit's a message event.

test('test 2 assertions will timeout', { timeout: 2000 }, async t => {
  const assert = assertPlan(1);

  chatbot.addEventListener('message', (message) => {
    assert.strictEqual(message.content, 'hello world');
  })

  await assert.wait();
});

But all you'll get in the logs are:

TAP version 13
# Subtest: test
not ok 1 - test
  ---
  duration_ms: 2.004530137
  failureType: 'testTimeoutFailure'
  error: 'test timed out after 2000ms'
  code: 'ERR_TEST_FAILURE'

But really, it would be helpful for me to somehow say:

assertPlan expected 1 assertion, but received 0 by the time the test timed out.

@markwylde
Copy link
Author

I guess a solution could be the ensure we never use the test runners timeout, and rely soley on assert-plan's timeout.

It feels wrong to me, but I think I'd be easily convinced otherwise.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 4, 2022

Thanks for the detailed issue. Just a few observations:

  • If you're worried about an assertion not being run, in my experience, it's because some callback/handler function was never called. Check out assert.CallTracker(). This is how Node's own test suite handles the problem. I'd say it's also less maintenance burden than plan() which needs to be updated when assertions are added/removed.
  • Node's assertion library throws immediately on a failed assertion, so the plan will be wrong on failing tests. For passing tests, see my previous point on an alternate way to handle that.
  • If we implemented plan() we'd also probably need to wrap the assert module so that failed assertions don't throw. That would also make it more difficult to bring your own assertion library, and so we'd probably need a generic API to hook into arbitrary assertion libraries.
  • I don't think we should expose the timer that the test uses internally, since user code could tamper with it.
  • before(), beforeEach(), after(), and afterEach() hooks landed recently, but haven't been released yet. You might be able to leverage those as well.

@benjamingr
Copy link
Member

We expose the AbortSignal in the test which has the event you describe and you can use.

On the other hand I do see value in exposing common.mustCall/common.mustNotCall we use extensively in our own tests maybe?

@MoLow
Copy link
Member

MoLow commented Aug 4, 2022

On the other hand I do see value in exposing common.mustCall/common.mustNotCall we use extensively in our own tests maybe?

that is already possible - just combinie assert.CallTracker() with process.on('exit').
that combination is actually the first example on the very beginning of CallTracker docs

@MoLow
Copy link
Member

MoLow commented Aug 4, 2022

also, implementing a plan for testing (any plan, not just for assersions) can be done with a counter and checking it inside a after hook

@aduh95 aduh95 added the test_runner Issues and PRs related to the test runner subsystem. label Aug 4, 2022
@MoLow
Copy link
Member

MoLow commented Aug 5, 2022

can this issue be closed?

@markwylde
Copy link
Author

Thank you everyone for taking the time to help me and come up with so many ideas/suggestions. I really appreciate it.

The before/after hook seems like a great way to solve this. I'll try it once it get's published.

@simoneb
Copy link
Contributor

simoneb commented Mar 18, 2024

Hi folks, apologies for resurrecting an old issue but we find ourselves needing the assestion planning feature that exists in most testing framework such as tap and jest, and which is hard to replicate without native support from the test runner.

Here's a simple example of something you'd write in tap:

test('plan', t => {
  t.plan(1)

  doSomething(() => t.ok(true))
})

Rewriting this test without native support for assertion planing is tricky. One way would be to create a promise:

test('no plan', t => {
  let resolve
  const p = new Promise(_resolve => { resolve = _resolve })

  doSomething(() => {
    t.ok(true)
    resolve()
  })

  return p
})

Clearly, this gets complicated and unfeasible very quickly, e.g. imagine nested callbacks, and has no easy user-space solution.

As much as I dislike test planning, because it makes tests harded to write and change, it's the only sensible way that I'm aware of to support assertions in callbacks, which are in some cases unavoidable.

Thoughts?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 18, 2024

I have a branch implementing this exact thing, but was on the fence as to whether or not to PR it. It would be good to hear what others think since a fair amount of time has passed since this issue was originally opened.

@simoneb
Copy link
Contributor

simoneb commented Mar 18, 2024

I have a branch implementing this exact thing, but was on the fence as to whether or not to PR it. It would be good to hear what others think since a fair amount of time has passed since this issue was originally opened.

That's a good start and good to hear, thanks @cjihrig. I assume that this also includes wrapping the assert module so it plays well along with node:test, right?

Personally, I'd say that 90% of the test I come across don't need this, but when you do, not having this feature is a show stopper.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 18, 2024

I assume that this also includes wrapping the assert module so it plays well along with node:test

That's correct. Here is some code from my branch:

test('plan passes with callbacks', (t, done) => {
  t.plan(3);

  t.assert.ok(true);
  setImmediate(() => {
    t.assert.ok(true);
    done();
  });
  t.assert.ok(true);
});

And, I haven't implemented it yet, but I designed this such that in the future we could support:

  • Using a different assertion library / adding additional assertion functions.
  • Silent assertions (similar to how they work in tap).

@simoneb
Copy link
Contributor

simoneb commented Mar 18, 2024

That's the way I would imagine it would look like. Now if it also worked, it would be amazing ;)

@MoLow
Copy link
Member

MoLow commented Mar 19, 2024

@simoneb
Copy link
Contributor

simoneb commented Mar 19, 2024

@simoneb have a look at https://github.com/mcollina/tspl /CC @mcollina

Thanks, I wasn't aware that Matteo built one. I'm sure there are alternatives, it would be great to pick one and bundle it in node:test.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 19, 2024

This is all it really takes to build it in: cjihrig@89d9a7e. Unlike a userland solution, this can also be easily extended to address #52033 (silent node-tap style assertions).

@mcollina
Copy link
Member

The fundamental reason I needed tspl was supporting Node 18 too.

@Tina1L

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants