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

Simplify failure output - fixes #1072 #1234

Merged
merged 3 commits into from
Feb 14, 2017
Merged

Simplify failure output - fixes #1072 #1234

merged 3 commits into from
Feb 14, 2017

Conversation

LasaleFamine
Copy link
Contributor

@LasaleFamine LasaleFamine commented Feb 6, 2017

The idea is to check when the test.error.source.file is NOT the same of the test.file within the forEach of the errors.

I saw that when I throw an error from another module the test.file refers to the module's file and not the file that is running the test.

In this case for example if I make this check, I'm able to hide the stack trace.

  1 failed

  lol.js:2

   1: const testing = () => { 
   2:   throw new Error('lol')
   3: }                       

  Error: lol

  testing (lol.js:2:8)
  Test.t [as fn] (test.js:5:2)

Waiting for your feedback to move forward!

@novemberborn
Copy link
Member

I saw that when I throw an error from another module the test.file refers to the module's file and not the file that is running the test.

To clarify, you mean test.error.source.file refers to the module's file?


Your current approach means no stack traces are ever shown for errors that come out of the test file. I think that's an issue since users may write helper functions in the test file itself.

Instead we should not output the stack when it only contains one line. So this should show the stack trace:

const testing = () => {
  throw new Error('lol')
}

test('testing', t => {
  testing()
})

But this should not:

test('another test', t => {
  throw new Error('lolol')
})

That's my interpretation of @sindresorhus' comment #1072 (comment).

@LasaleFamine
Copy link
Contributor Author

LasaleFamine commented Feb 7, 2017

To clarify, you mean test.error.source.file refers to the module's file?

I this case (that will produce mi first post output), test.error.source.file === 'lol.js' but test.file === 'test.js':

// test.js
import test from 'ava'
import fn from './lol'

test('asd', t => {
    fn()
})
// lol.js
module.exports.testing = () => {
    throw new Error('lol')
}

I understood your point, actually initially I was thinking to check if the extracted stack trace was only one line, but I didn't know if that single line could contain some other info.
I can proceed in this way if you agree.

@novemberborn
Copy link
Member

I understood your point, actually initially I was thinking to check if the extracted stack trace was only one line, but I didn't know if that single line could contain some other info.
I can proceed in this way if you agree.

Yes I think that's the way to go.

@LasaleFamine
Copy link
Contributor Author

Can I assume that there aren't \n chars within the extracted stack?

Ex:

// lib/reporters/mini.js finish(runStatus)

// Check also if the stack isn't only the `test Fn` (actually check for one-line stack)
if (test.error.stack && extractStack(test.error.stack).search('\n') > -1) {
       status += '\n' + indentString(colors.errorStack(extractStack(test.error.stack)), 2);
}

Actually I don't like so much this solution because I'm calling twice the same function and I really need the comment to explain what I'm doing.
Do you have a better/correct solution for the problem?

@novemberborn
Copy link
Member

Actually I don't like so much this solution because I'm calling twice the same function and I really need the comment to explain what I'm doing.

You could assign the stack to a variable:

if (test.error.stack) {
  const extracted = extractStack(test.error.stack);
  if (extracted.includes('\n')) {
    status += '\n' + indentString(colors.errorStack(extracted), 2);
  }
}

@LasaleFamine
Copy link
Contributor Author

I have made the change and also added the check to the verbose reporter. Also test fixed.
Let me know if I need to change something.

@novemberborn
Copy link
Member

screen shot 2017-02-09 at 12 02 56

This is the output for the examples I posted in #1234 (comment). Looks good to me! @sindresorhus?

@sindresorhus sindresorhus changed the title Simplify failure output #1072 Simplify failure output - fixes #1072 Feb 14, 2017
@sindresorhus sindresorhus merged commit da68f29 into avajs:master Feb 14, 2017
@sindresorhus
Copy link
Member

Looks great! Thank you @LasaleFamine :)

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.

3 participants