nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.89k stars 29.16k forks source link

Test runner reports duplicate titles #54587

Closed nicholaswmin closed 1 month ago

nicholaswmin commented 1 month ago

Version

22

The default [`spec` test reporter](https://nodejs.org/api/test.html#test-reporters) prints duplicate test titles for some reason.   
I guess that's expected but not clear why, nor documented. 

I write my tests in a way that resembles Cucumber BDD specs - reading it back is essential to making sure I've covered
my requirements. This duplication makes it awkward to read.

Subsystem

test-runner

What steps will reproduce the bug?

Run this with node --test:

import test from 'node:test'

test('test', async t => {
  await t.test('if its ok or fine', async t => {    

    await t.test('is ok',  t => {
      t.assert.ok(true)
    })

    await t.test('is fine', t => {
      t.assert.ok(true)
    })

  })
})

Output

▶ test
  ▶ if its ok or fine   <-- printed title
    ✔ is ok (0.454084ms)
    ✔ is fine (0.051625ms)
  ▶ if its ok or fine (0.834375ms)   <-- same title printed 
▶ test (1.145416ms)
ℹ tests 4
ℹ suites 0
ℹ pass 4
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 52.01175

How often does it reproduce? Is there a required condition?

everytime

What is the expected behavior? Why is that the expected behavior?

▶ test
  ▶ if its ok or fine   <-- printed title
    ✔ is ok (0.454084ms)
    ✔ is fine (0.051625ms)
▶ test (1.145416ms)
ℹ tests 4
ℹ suites 0
ℹ pass 4
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 52.01175

What do you see instead?

▶ test
  ▶ if its ok or fine   <-- printed title
    ✔ is ok (0.454084ms)
    ✔ is fine (0.051625ms)
  ▶ if its ok or fine (0.834375ms)   <-- same title printed 
▶ test (1.145416ms)
ℹ tests 4
ℹ suites 0
ℹ pass 4
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 52.01175

Additional information

No response

puskin94 commented 1 month ago

@nicholaswmin Hi! I guess you see it printed twice ( I didn't take a look at the code yet ) because:

  1. the first time you see it is just informing you about the suite of tests it is going to run
  2. the second time is the "final result" , once the suite is done, and it contains the total execution time of all the sub tests

I don't think this is an issue! But I might be wrong 😄

nicholaswmin commented 1 month ago

Aham I see - well, my vote is to ditch the double title and just print the timing. Close if needed.

RedYetiDev commented 1 month ago

the second time is the "final result" , once the suite is done, and it contains the total execution time of all the sub tests

That is indeed what is occurring.

puskin94 commented 1 month ago

the second time is the "final result" , once the suite is done, and it contains the total execution time of all the sub tests

That is indeed what is occurring.

Do you think what is suggested is worth implementing? I could give it a go

RedYetiDev commented 1 month ago

@nodejs/test_runner will have better insight than I.

nicholaswmin commented 1 month ago

I see the spec reporter name everywhere, sounds like there could be a "spec" for it and if so what does it say about the format? Is it standardised?

RedYetiDev commented 1 month ago

AFAIK there is not a spec for it, but here are a few other implementations:

(Example from WebDriverIO's documentation) WebDriverIO

(Example from Mocha's documentation) Mocha

puskin94 commented 1 month ago

I actually took a look into the code and it would quite difficult to apply the suggestion!
The main problem is that, for each test, the line in the test report is immediately printed as soon as the test is done. What does this mean?

image

test "is ok" prints almost immediately but the "is fine" is only printing after more than 2 seconds (I caused a delay). This means that when we print the "test" line (the test suite containing the other tests) we don't know YET how long the sub-tests will take for the execution.
There are ways to rewrite a line in a terminal using libraries like ncurses and such but that would require quite a lot of work for a very small quality of life improvement, IMHO 😄

We could also re-arrange how lines are displayed, for example putting the "test" line after the sub-tests, making sure that we already know how much all the previous tests took, but I guess that would also upset other people for stylistics reasons 😄

nicholaswmin commented 1 month ago

Yes that's the issue - my suggestion is to just print the execution time without a double title.

Doable - just remove the duplicate title printing and that's it.

I'm certain spec is a format. Let me dive a bit into it.

nicholaswmin commented 1 month ago

AFAIK there is not a spec for it, but here are a few other implementations:

(Example from WebDriverIO's documentation) WebDriverIO

(Example from Mocha's documentation) Mocha

Now this - this is pretty, nice, readable, yes. This is how the gods intended a spec report should look like.
I come from a long Mocha background and this is bueno.

That being said - both look clean simply because they lack timing information.

puskin94 commented 1 month ago

Yes that's the issue - my suggestion is to just print the execution time without a double title.

Doable - just remove the duplicate title printing and that's it.

You could remove the one without the execution time, but then you have some weird indentation, where the sub tests are printed before the test suite name, and indented more

nicholaswmin commented 1 month ago

You could remove the one without the execution time, but then you have some weird indentation, where the sub tests are printed before the test suite name, and indented more

Wait a second - why not remove the one with the timing info - but keep the timing info? Maybe ident it so it's grouped under the test group.

Like this:

▶ test
  ▶ if its ok or fine   <-- printed title
    ✔ is ok (0.454084ms)
    ✔ is fine (0.051625ms)
    ▶ (0.834375ms)  
▶ test (1.145416ms)
ℹ tests 4
ℹ suites 0
ℹ pass 4
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 52.01175
puskin94 commented 1 month ago

Wait a second - why not remove the one with the timing info - but keep the timing info? Isn't it indented under the title anyway?

so you would JUST remove the title of the test, but keeping the time exactly where it is? That's a weird one, but doable . In fact, I already have the code ready, I am just not sure if I like how it looks like tbh... it looks... weird

nicholaswmin commented 1 month ago

Wait a second - why not remove the one with the timing info - but keep the timing info? Isn't it indented under the title anyway?

so you would JUST remove the title of the test, but keeping the time exactly where it is? That's a weird one, but doable

The title of the test is printed once anyway. I'm not removing the title of the test - i'm removing the duplicate title. The title stays where it is.

not sure if I like how it looks like tbh... it looks... weird

Can you paste a snippet of how it looks?

puskin94 commented 1 month ago
image

this would be the final result, and I really don't think we should do it, for a very simple reason:

the example you posted is extremely simple, you have 1 test suite and 2 sub tests. Now, let's think for a second you have hundreds or thousands of tests suites with the same amount of sub tests; if I was a developer interested in knowing which suite is taking a long time to execute and I would see such a report? I would be a really sad developer 😄 I would have to understand which suite took that amount of time scrolling up in the terminal at the test suite with the same indentation as the time, and remember, you have so many tests and test suites, all nested 😄

RedYetiDev commented 1 month ago

+1 on not doing that. I'm pretty okay with the test runner format with or without the duplicate title, however removing the duplicate title requires a significant rewrite of the spec reporter.

nicholaswmin commented 1 month ago

the example you posted is extremely simple, you have 1 test suite and 2 sub tests. Now, let's think for a second you have hundreds or thousands of tests suites with the same amount of sub tests; if I was a developer interested in knowing which suite is taking a long time to execute and I would see such a report? I would be a really sad developer 😄 I would have to understand which suite took that amount of time scrolling up in the terminal at the test suite with the same indentation as the time, and remember, you have so many tests and test suites, all nested 😄

All good points, can't disagree - it does look cluttered though.

Close if needed.

nicholaswmin commented 1 month ago

For the record this spec reporter diverges from what I know as spec reporters. It feels like it was imagined by someone who only worked with tap; not saying that's bad since it's informative but how comes and it diverges from the canonical spec examples mentioned above?

On the other hand who's to say what a spec should be? there's no "spec" specification i could find.

If this was my decision and my decision only, I'd do something like this: mocha-better-spec-reporter

image

Anyway, I think this needs to be closed. Maybe this is another ticket, maybe not. 👍🏼 Thanks everyone

RedYetiDev commented 1 month ago

In reality, this is a request for a change of a feature, so I've updated the labels

cjihrig commented 1 month ago

Anyway, I think this needs to be closed

I'm going to close this issue. For anyone motivated to work on improving the reporters - feel free. I don't think anyone feels very strongly about the current format or would be opposed to trying to improve it. We also don't make any semver guarantees about the exact output of the reporters, so we have a bit of freedom there.

Also, if you're just looking for something that looks like mocha, check out https://www.npmjs.com/package/@reporters/mocha.