chore: improve the test reporter's messaging #5724
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
galargh
changed the title
galargh
force-pushed
the
galargh/test-reporter-ui
branch
from
a65bcb5 to
bc0a738
Compare
This is a much better experience.
yay!
Makes sense. See below my other comment about this.
I think this change is needed, but this implementation breaks the usage of this reporter outside of hardhat (e.g. in our own tests). I think we can have two exports in the package:
This will be helpful in a comment below.
Makes sense
This is great. Much cleaner.
Great idea!
Unfortunately, this doesn't work, and we need to support that case. The reason is that our users will definitely do that. For example: This already happens in practice, I'm trying to get Mocha to show an error in those situations, as they don't support async describes, but node does. I went deeper into the reporter's code, and I understand when this happens and how to detect it: /**
* Returns true the event represents a failure due to trying to create a subtest
* when its parent has already finished.
*
* This normally happens if you are running in `only` and the parent is async,
* as the parent gets cancelled but continues running an async operation, and
* try to create a subtests after it.
*/
export function isSubtestCreationAbortedError(event: TestEvent): boolean {
if (event.type !== "test:fail") {
return false;
}
const error = event.data.details.error;
if (!("code" in error) || !("failureType" in error)) {
return false;
}
return (
error.code === "ERR_TEST_FAILURE" &&
error.failureType === "parentAlreadyFinished"
);
}It may also be generated when the users forget an What if we do this?
(we would need to add tests for this) |
|
Also, let's revert the change to our test that does that. If we are going to support it, it's better to dogfood it. |
…nup' into galargh/test-reporter-ui
…nup' into galargh/test-reporter-ui
| // We need to subtract the number of tests/suites that we chose not to | ||
| // display from the summary | ||
| if (event.data.message.startsWith("tests")) { | ||
| const parts = event.data.message.split(" "); | ||
| const count = | ||
| parseInt(parts[1], 10) - | ||
| topLevelFilePassCount - | ||
| parentAlreadyFinishedFailureCount; | ||
| event.data.message = `${parts[0]} ${count}`; | ||
| } | ||
| if (event.data.message.startsWith("pass")) { | ||
| const parts = event.data.message.split(" "); | ||
| const count = parseInt(parts[1], 10) - topLevelFilePassCount; | ||
| event.data.message = `${parts[0]} ${count}`; | ||
| } | ||
| if (event.data.message.startsWith("fail")) { | ||
| const parts = event.data.message.split(" "); | ||
| const count = | ||
| parseInt(parts[1], 10) - parentAlreadyFinishedFailureCount; | ||
| event.data.message = `${parts[0]} ${count}`; | ||
| } |
There was a problem hiding this comment.
Maybe all this logic should go to processGlobalDiagnostics
There was a problem hiding this comment.
That's a good catch. I now moved it to after processGlobalDiagnostics which I think is much cleaner, see 8332321 (#5724)
| // We still display the failed suites, because failures for the nested | ||
| // tests (cancelled by parent errors) were already displayed. |
There was a problem hiding this comment.
This doesn't work well, as it looks like an error. I think a good trade-off would be to detect that situation and maybe display those canceled tests/suites as a particular case in the summary at the end of the test run if needed.
This is a complex case, so we can continue addressing it in a follow up PR.
There was a problem hiding this comment.
I replied in a longer form in #5724 (comment) TLDR I removed any attempt to handle that case from this PR + the newest node makes the output for these cases a lot better (e.g. in hardhat package, test:only works as expected now).
|
I'll look closer at it tomorrow and prepare the PR for a re-review properly, but I reverted any custom handling for the async describe case and the diff from node v22.8.0 looks quite promising - 5599b92 |
I think this was a great suggestion and I implemented it straight away. Now we have the following 2 exports:
After the experimentation and discussions we had on Slack, I do agree that silencing might not be the way forward in this case. Actually, I don't think we should do that even when running in the only mode. I now reverted the changes related to that silencing "parent already finished" errors.
As agreed, this PR will not introduce any special behaviour for "parent already finished" errors as it's a complex issue without a clearly right way forward. We'll deal with that in a separate PR if needed. One thing worth noting is that some of the issues we were dealing with were addressed in This should be the result of nodejs/node#54423 as far as I can tell. With that PR in place, we can only see the "parent already finished" errors in nested async describes. Something like: describe("a suite", () => {
// If we remove this, no tests will be executed and no failures will be reported.
it.only("an only test", async () => {
assert.equal(1, 1);
});
describe("a nested suite with an async callback", async () => {
const one = await new Promise((resolve) => setTimeout(() => resolve(1), 0));
it("a nested test", async () => {
assert.equal(one, 1);
});
it.only("a nested only test", async () => {
assert.equal(one, 1);
});
});
});The issue is also only visible (reported by the test runner) if there is some block marked as only at the same level as the nested async describe. All in all, triggering the issue requires a muuuch more complex setup now. We could consider whether trying to handle that edge case is worth pursuing. I would suggest re-reviewing the PR with whitespace changes hidden - https://github.com/NomicFoundation/hardhat/pull/5724/files?w=1 This is because of all the indentation changes in the |
Please note that this PR is currently based on #5714
This PR implements improvements to our test reporter that were discussed in and around #5669 and #5692
Here's a breakdown of the proposed changes together with links to their UI diffs (if you prefer me to break the PR down into separate ones, please let me know):
--onlyflag. It is introduced in 270f889ERR_TEST_FAILUREfrom the formatted error details. Similarly to how we improve the assertion error message. It is introduced in bfb1415Additionally, I removed the usage of async describe block from hardhat tests since it doesn't work well with node's test reporter even though it is supported (fee6fb5). We could think about making it into a linting rule, maybe?