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

Include failed test files and titles in default reporter #2647

Closed
wants to merge 1 commit into from

Conversation

stavalfi
Copy link
Contributor

@stavalfi stavalfi commented Jan 15, 2021

Related to #2500 (comment)

@novemberborn I want to hear your opinions before fixing the tests.

Difference when running Ava on one of my personal projects:

Before:

image

After:

image


The only way to search the failed tests before this PR is to search the "✖" symbol and search for "Uncaught exception".


TODO:

@novemberborn When all of the tests of a worker passed but the worker exit with code 1, I can't find a way to get the failed-tests in the default reporter:

test('test1', async () => {
  process.exitCode = 1
})

In this situation, I would also expect that --serial --fail-fast won't run tests after test1 but that's not the case. Maybe Ava needs to evaluate the process.exitCode before moving on to the next test.

wdyt?

@novemberborn
Copy link
Member

Apologies I've lost track of what this is solving. Surely we are reprinting the failed tests already? What am I missing?

(And yes my confusion is a sure sign this code needs to be replaced.)

There should be some message for the worker crashing already I would think.

With --fail-fast, new tests shouldn't be run, though everything already in motion will continue running.

@stavalfi
Copy link
Contributor Author

stavalfi commented Jan 17, 2021

@novemberborn Thanks for the great feedback.

Surely we are reprinting the failed tests already? What am I missing?

As you can see in the screenshots, what I have printed is not printed.

reprinting the failed tests

Ava does prints the failed tests logs again. in case there are tons of logs for each test, it won't help that much to get an instant summary of what has failed.

Also, at least in a specific scenario, the failed tests are not printed at all:

image

I don't remember the reason behind this specific screenshot.


I have tested the scenario where:

test('test1', async () => {
  process.exitCode = 1
})

The results are the following:

  1. Tests are keep running (even with --serial --fail-fast).
  2. With --serial, there is no clear summary of what test-title failed.
  3. With or without any extra flag, there is no clear summary of what test-file failed.

NOTE: This PR doesn't solve this. I'm not sure if it should be on this PR or not.


I just want to mention that if you are confused about something, we can be sure that I'm missing something.

Maybe you can point me out to the right direction?

Thanks again!

@novemberborn
Copy link
Member

I have tested the scenario where:

test('test1', async () => {
  process.exitCode = 1
})

The results are the following:

  1. Tests are keep running (even with --serial --fail-fast).
  2. With --serial, there is no clear summary of what test-title failed.
  3. With or without any extra flag, there is no clear summary of what test-file failed.

NOTE: This PR doesn't solve this. I'm not sure if it should be on this PR or not.

Let's discuss that separately.


Ava does prints the failed tests logs again. in case there are tons of logs for each test, it won't help that much to get an instant summary of what has failed.

I was thinking of this code:

this.writeFailure(event);

But then there's other errors that are not printed in verbose mode… perhaps because it would be repetitive, perhaps because of forgotten reasons:

if (!this.verbose) {

I just want to mention that if you are confused about something, we can be sure that I'm missing something.

Hehe, I appreciate that, but the reporting code is quite confusing. It's grown based on feedback and is no longer based on clear intent.

@novemberborn
Copy link
Member

Closing due to inactivity. We'd be happy to pick this up again when you have the time to get back to this.

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

Successfully merging this pull request may close these issues.

2 participants