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

Subtests cause the parent test to stop showing up as green even if it asserts. #4482

Closed
nicholaswmin opened this issue Sep 15, 2024 · 9 comments

Comments

@nicholaswmin
Copy link

nicholaswmin commented Sep 15, 2024

v22.8

Creating subtests reports the parent as if they on't test anything are only a placeholder.

In the following, all the test assert something.
The output makes it appear as if only:

✔ with a name
✔ in the correct format

actually asset and have passed.


I follow the below pattern when writing tests.

If I'm asserting something deep I drill down to it contextually, step by step, to avoid duplication, because I think tests should be as concise and easy to digest as possible. it's a recommended pattern to group tests as far as I know.

The spec reporter stops showing a test as "green" when it contains subtests, even though it does assert.

This might sound pedantic but the I treat my tests as a .. spec. If I'm asserting something I want to have visual feedback in 6 months that I'm checking that title and it does pass which gives me a hint that's a valid use-case I've covered.

I routinely end up with modules with > 150 tests and the spec reporter output looks.. anemic I suppose?

Is there a reason for this?

Hope tihs makes sense.

Minimal Reproduction

import test from 'node:test'


test('Returns a parent with and its children', async t => {
  const parent = { name: 'foo', children: ['bar', 'baz'] }
  
  await t.test('is an object', async t => {
    t.assert.strictEqual(typeof parent, 'object')
    t.assert.notStrictEqual(parent, null)

    await t.test('with a name', async t => {
      t.assert.ok(Object.keys(parent).includes('name'), '"name" prop. missing')
      t.assert.notStrictEqual(parent.name.length, 0, 'name is empty ')
    })
    
    await t.test('and some children', async t => {
      t.assert.ok(Object.keys(parent).includes('children'))

      await t.test('in the correct format', t => {
        t.assert.ok(Array.isArray(parent.children), 'parent.child !== Array')
        t.assert.notStrictEqual(parent.children.length, 0, 'no children')
      })
    })
  })
})

Output

Current Output:

▶ Returns a parent with and its children
  ▶ is an object
    ✔ with a name (0.445583ms)
    ▶ and some children
      ✔ in the correct format (0.060583ms)
    ▶ and some children (0.192375ms)
  ▶ is an object (1.092125ms)
▶ Returns a parent with and its children (1.47275ms)

Expected Output

▶ Returns a parent with and its children
  ✔  is an object
    ✔ with a name (0.445583ms)
    ✔ and some children
      ✔ in the correct format (0.060583ms)
    ✔ and some children (0.192375ms)
  ✔ is an object (1.092125ms)

I would expect the current output to match something like this, without assertions in the parents, which I think is unnecessary nesting, unless there are assertions in the parent as explained above.

test('Returns a parent with and its children', async t => {
  const parent = { name: 'foo', children: ['bar', 'baz'] }
  
  await t.test('is an object', async t => {
    await t.test('with a name', async t => {
      t.assert.ok(Object.keys(parent).includes('name'), '"name" prop. missing')
      t.assert.notStrictEqual(parent.name.length, 0, 'name is empty ')
    })
    
    await t.test('and some children', async t => {
      await t.test('in the correct format', t => {
        t.assert.ok(Array.isArray(parent.children), 'parent.child !== Array')
        t.assert.notStrictEqual(parent.children.length, 0, 'no children')
      })
    })
  })
})
@RedYetiDev
Copy link
Member

When you say "green", you are referring to the color, right?

You want tests with sub-tests to be printed green when all subtests pass?

@nicholaswmin
Copy link
Author

When you say "green", you are referring to the color, right?

almost... yes, this is a visual change

I want any test that has assertions that pass to be marked as clearly and unambigously as "passed" - ideally yes, using the same visual pattern as the rest of the passing tests, via a tick and the color green.

The problem is that neither of those effects take effect when a test has subtests.
The following can both be true:

  • a test has subtests
  • a tests is asserting something in-and-by-itself. Not it's subtests, although that can also be true.

In the following example:

 Returns a parent with and its children
   is an object
     with a name (0.445583ms)
     and some children
       in the correct format (0.060583ms)
     and some children (0.192375ms)
   is an object (1.092125ms)
 Returns a parent with and its children (1.47275ms)

only:

✔ with a name (0.445583ms)
✔ in the correct format (0.060583ms)

are actually marked as passed.

However, all tests have assertions, they just also happen to have subtests. Therefore all tests should be marked as passed not just those 2.

The identation level is enough to indicate they have subtests. A tests shouldn'y lose it's "passing" visual status because it has subtests, unless it does not have any assertion itself.

Does that make sense - I understand it's a bit of mind.. bend.

@nicholaswmin
Copy link
Author

nicholaswmin commented Sep 15, 2024

You want tests with sub-tests to be printed green when all subtests pass?

No, just to clarify - that statement means that this would pass, clearly it does not and correctly so.

  const parent = { name: 'foo', children: ['bar', 'baz'] }
  
  await t.test('is an object', async t => {
    t.assert.strictEqual(typeof parent, 'number') // <-- not a number

    await t.test('with a name', async t => {
      t.assert.ok(Object.keys(parent).includes('name'), '"name" prop. missing')
    })
  })

Let's fix the above test:

test('Returns a parent with and its children', async t => {
  const parent = { name: 'foo', children: ['bar', 'baz'] }
  
  await t.test('is an object', async t => {
    t.assert.strictEqual(typeof parent, 'object') // <-- ok, should pass now

    await t.test('with a name', async t => {
      t.assert.ok(Object.keys(parent).includes('name'), '"name" prop. missing')
    })
  })
})

and run it:

▶ Returns a parent with and its children
  ▶ is an object <-- this has a passing assertion, not marked as passed visually
    ✔ with a name (0.386166ms)
  ▶ is an object (0.76725ms)
▶ Returns a parent with and its children (1.10075ms)

so I want it to output:

▶ Returns a parent with and its children
  ✔ is an object <-- much better
    ✔ with a name (0.386166ms)
  ▶ is an object (0.76725ms)
▶ Returns a parent with and its children (1.10075ms)

@nicholaswmin
Copy link
Author

oops, reopening

@RedYetiDev
Copy link
Member

RedYetiDev commented Sep 15, 2024

Got it. I understand, and I'll take a look later today.

@nicholaswmin
Copy link
Author

nicholaswmin commented Sep 15, 2024

Ah, mind just poitning me to the test runner source so I can take a look? It's time that I get a bit up to speed with the internals, i owe a PR.

Given that's ok of course - I see that not everyone has commit access

@RedYetiDev
Copy link
Member

Given that's ok of course - I see that not everyone has commit access

You can still submit PRs. You just can't modify main directly.

The test runner source is available at lib/internal/test_runner. In this case, lib/internal/test_runner/reporters.

@RedYetiDev
Copy link
Member

See nodejs/node#54956

@nicholaswmin
Copy link
Author

@RedYetiDev Yes, LGTM. Simple. I like it

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

Successfully merging a pull request may close this issue.

2 participants