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

Default reporter swallows console.log #392

Closed
TrySound opened this issue Dec 31, 2015 · 16 comments
Closed

Default reporter swallows console.log #392

TrySound opened this issue Dec 31, 2015 · 16 comments
Assignees
Labels

Comments

@TrySound
Copy link
Contributor

test.js

import test from 'ava';

test('log test', t => {
    console.log('log1\nlog2');
});

terminal output

log1

  1 passed

Seems like the last string of logged text is swallowed.It complicate debuggin.

@sindresorhus sindresorhus changed the title swollowed output Default reporter swallow console.log Jan 1, 2016
@sindresorhus
Copy link
Member

I noticed this too. This is because of the animated reporting. Although, logging like that isn't very useful when you have more than one tests as the logging is synchronous while the test is not, so the logging could be outputted with a different test. I think a solution here is to handle the logging so that it's outputted with the correct test in verbose mode and with the default minimal reporter we could either show all logging at the end or during the testing by pushing the animated output down (meaning calling logUpdate.done()).

For now you can use the --verbose flag.

// @vdemedes

@jamestalmage
Copy link
Contributor

I think a solution here is to handle the logging so that it's outputted with the correct test in verbose mode

That would be hard to do with console.log unless we either:

  1. Force --serial when --verbose is used (I don't like this idea).
  2. Use domains to determine which test console.log is used from.
  3. Provide a t.log method.

I think 3 is pretty interesting. It won't help much with capturing logs from production code (unless you use proxyquire to inject it some how). For the verbose reporter, we could always print t.log statements. For the mini reporter, only output logs with failed tests

@sindresorhus
Copy link
Member

Yes, 3 is what I was thinking.

For the mini reporter, only output logs with failed tests

I think we should still log it with the mini reporter as it's still useful to quickly print out some values.

@ariporad
Copy link
Contributor

ariporad commented Jan 2, 2016

@sindresorhus, @jamestalmage: We could also try hijacking/monkey-patching wither process.stdout/stderr or console.log.

@sindresorhus
Copy link
Member

Yes, we could, but I have a feeling that comes with a lot of edge-cases, so we should first figure out t.log.

@ecowden
Copy link

ecowden commented Jan 2, 2016

Humble lurker here, so feel free to tell me I'm wrong --

Usually, when I find myself dropping in log statements for debugging, it's in the code under test, not the test itself. If the data I'm trying to learn about is accessible from the test, I'd just write an assertion anyway. I doubt I'd use t.log very much.

I've found debugging particularly tricky when experimenting with AVA since it also seems to give debuggers like node-inspector a hard time. In an ideal world, I'd love to see the log output for failed tests alongside the failed assertion or error. Successful tests could hide logs unless handed --verbose. Of course, easier said than done, right?

@vadimdemedes
Copy link
Contributor

I think we should just hide the output from the mini reporter. I've encountered many TAP/mocha reporters, that just don't show console.log()s and that's it. With our animated beauty, there is simply no good way to display console.log() output.

I doubt I'd use t.log very much.

Highly agree with @ecowden, I don't think anyone would use t.log.

@TrySound
Copy link
Contributor Author

TrySound commented Jan 2, 2016

But how to debug?

@jamestalmage
Copy link
Contributor

use --verbose

@vadimdemedes
Copy link
Contributor

@jamestalmage++

@jamestalmage
Copy link
Contributor

I don't think anyone would use t.log

Yeah, that was my concern as well. However, I think having a way to log output that you can be sure will be printed right underneath the ✅ of the related test is pretty valuable. Obviously --serial is a way to achieve that.

At some point I think we need to start developing a "best practices / hints and tips / faq" document with stuff like this in it (where we fail to find ideal / straightforward solutions to these types of problems). For example, a good heading for the discussion we are having here would be "Debugging async tests", that talks about the difficulty of debugging async tests (it's hard to reason about console.log statements unless you enable --serial).

@jamestalmage
Copy link
Contributor

For the mini reporter, only output logs with failed tests

I think we should still log it with the mini reporter as it's still useful to quickly print out some values.

@sindresorhus - for quickly printing out some values, console.log works just as well, assuming you intend to delete the log statement later. The output of the mini reporter offers no context for log statements in passing tests anyways. There are advantages to not displaying in the mini reporter unless there is a failure:

  1. The log statements can be given context (by being grouped with the failure output for that specific test).
  2. You can leave the log statements in the test. As long as the test passes, it is not cluttering up your output. If/when the test fails - you are given additional information beyond the simple assertion failure.

@sindresorhus
Copy link
Member

The output of the mini reporter offers no context for log statements in passing tests anyways.

Usually I just console log in one place so I don't really need a context, but ok with using --verbose when I do that.

@jamestalmage
Copy link
Contributor

Usually I just console log in one place so I don't really need a context

In that case the mini reporter should be fine. Though maybe we should somehow use hook-std or something to detect a line has been written and avoid deleting it.

@sindresorhus
Copy link
Member

Yup, already suggested something like that initially, using logUpdate.done(). Would be nice.

@jamestalmage jamestalmage self-assigned this Jan 9, 2016
jamestalmage added a commit to jamestalmage/ava that referenced this issue Jan 9, 2016
…vajs#392.

This fixes two errors with our logging/io operations:

 1. We were using the `stdio` option for `child_process.fork`, but that option is ignored (it is honored for just about every other method in that API).

 2. The mini reporter uses logUpdate which deletes lines previously written with the logUpdate API before writing a new one. This caused problems if lines from console.log were written inbetween logUpdate calls. It would delete the users log messages instead of the last test title. To fix this, we store the last written logUpdate line, clear it, write the users log output, then restore the logUpdate line. This keeps the miniReporter output always at the bottom of the log output.

Reference:

 - https://github.com/nodejs/node/blob/7b355c5bb30d7f9b38654fdb50e58dbd1dcd990a/lib/child_process.js#L49-L50
@vadimdemedes vadimdemedes changed the title Default reporter swallow console.log Default reporter swallows console.log Jan 12, 2016
@Qix-
Copy link
Contributor

Qix- commented Jan 12, 2016

Yes, we could, but I have a feeling that comes with a lot of edge-cases

Not really. Patching process.stdout, process.stderr and fs.write(1|2, ...) could do it completely. The only thing that this wouldn't cover is native modules writing to either of those, though that's hard to cover anyway.

For the first two, you could even .pipe() them to a logger stream that AVA would provide. Would be much cleaner than an override.

Definitely not against t.log() though, however that doesn't stand up if the code being tested (not the tests themselves) have any sort of console output.

Not sure what #415 does (haven't looked into it yet) but I figured I'd jump here and comment. Ava is a standalone application that has the special ability to be able to set up hooks in the environment to do what it does (it's a testing framework, after all). I'm surprised we don't hook more things (side effect testing and whatnot; t.assertFileWritten(filename), etc).

jamestalmage added a commit to jamestalmage/ava that referenced this issue Jan 13, 2016
This prevents interference between the mini logger and child processes that use `console.log`. The mini reporter previously used logUpdate which deletes lines previously written with the logUpdate API before writing a new one. This caused problems if lines from console.log were written inbetween logUpdate calls. It would delete the users log messages instead of the test status line. To fix this, we store the last written last log line, clear it, write the users log output, then restore the last log line. This keeps the miniReporter output always at the bottom of the log output.

It also fixes an incorrect use of the `child_process` API. We were using the `stdio` option for `child_process.fork`, but that option is ignored (it is honored for just about every other method in that API). See: https://github.com/nodejs/node/blob/7b355c5bb30d7f9b38654fdb50e58dbd1dcd990a/lib/child_process.js#L49-L50

It also adds a set of visual tests which can be run via `npm run visual`. They must be run manually, and should be run as part of our pre-release process.
sindresorhus added a commit that referenced this issue Jan 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants