Add support for test logging #1452

nowells · 2017-07-13T14:56:05Z

There are times when you wish to include comments in your tests that will be shown in context of the test result and not streamed to stdout like console.log statements are, where they will likely be out of order of the tests they are associated with.

I will happily add tests, and change the API, etc. I just want to get general 👍 or 👎 to the concept of adding comments before I flesh out the tests and other cleanup.

One of the main use cases I have for this, is I am using ava as a runner for my SauceLabs/BrowserSrack/Selenium tests, and I would like to have the link to the job results from SauceLabs/BrowserStack be output in context with the test run. There are certainly many other usecases for this, however, just to give a relevant example of when this is useful.

Example output

`--verbose`

`--tap`

TAP version 13                                                                                                                                                              [30/1823]
# file-one › passing test
ok 1 - file-one › passing test
  a comment for passing test
# file-two › passing test
ok 2 - file-two › passing test
  a comment for passing test
# file-one › failing test
not ok 3 - file-one › failing test
  ---
    name: AssertionError
    assertion: is
    values:
      'Difference:': |-
        - true
        + false
    at: 'Test.t [as fn] (file-one.test.js:11:7)'
  ...
  a comment for failing test
# file-two › failing test
not ok 4 - file-two › failing test
  ---
    name: AssertionError
    assertion: is
    values:
      'Difference:': |-
        - true
        + false
    at: 'Test.t [as fn] (file-two.test.js:11:7)'
  ...
  a comment for failing test

1..4
# tests 4
# pass 2
# fail 2

nowells · 2017-07-13T16:46:03Z

lib/reporters/verbose.js

 		if (test.error) {
-			return '  ' + colors.error(figures.cross) + ' ' + test.title + ' ' + colors.error(test.error.message);


This whole stream of changes looks more noisy than it is. This is simply switching from an immediate return approach to appending to a lines.push approach so that we can concatinate comments after the test summary.

novemberborn · 2017-07-20T11:14:22Z

Looks like you've inadvertently implemented #420! 👍

This looks great. I think it should work with the (default) mini reporter too, since that still prints failing tests.

Bike shedding consensus from #420 was to use t.log() rather than t.comment().

nowells · 2017-07-20T12:51:34Z

@novemberborn thanks for the review! I will:

Switch from comment to log
Add log output to mini reporter on failure
Add tests for t.log

I should hopefully have this up to you for review by the end of the day. Cheers!

nowells · 2017-07-20T15:42:51Z

@novemberborn let me know if my rework looks good, and if you think there are other types of tests that I should add. Thanks!

sindresorhus · 2017-07-25T10:23:12Z

@nowells Can you document it in the readme?

sindresorhus · 2017-07-25T10:23:39Z

I think it should work with the (default) mini reporter too, since that still prints failing tests.

How would that work? The comments would need context.

nowells · 2017-07-25T12:52:12Z

Can you document it in the readme?

Absolutely! Done.

How would that work? The comments would need context.

The way I implemented it, in mini reporter the log messages show up in failure cases (arguably the only place you really care to see them in that context) but in success cases I do not display them.

novemberborn

This is almost there @nowells. Awesome work!

novemberborn · 2017-07-25T20:38:00Z

lib/reporters/tap.js

+		const appendLogs = () => {
+			if (test.logs) {
+				test.logs.forEach(log => {
+					output.push(`  ${log}`);


If log contains linebreaks, should the indentation apply on those new lines? If so, this should use https://www.npmjs.com/package/indent-string (like elsewhere in the reporters). May not be a bad idea to use that anyway.

should the indentation apply on those new lines?

Yes

Good catch! I'm going to make it so that it will result in something like this, where I add the i figure to the first line.

✔ file-one › passing test ℹ a comment for passing test with a newline ℹ a comment for passing test without a newline

novemberborn · 2017-07-25T20:48:03Z

lib/reporters/verbose.js

+
+				if (test.logs) {
+					test.logs.forEach(log => {
+						output += '    ' + colors.information(figures.info) + ' ' + colors.log(log) + '\n';


Same question here around linebreaks in the log value.

novemberborn · 2017-07-25T20:49:48Z

readme.md

@@ -875,6 +875,10 @@ Plan how many assertion there are in the test. The test will fail if the actual

 End the test. Only works with `test.cb()`.

+###### `t.log(message)`
+
+Print a log message contextually alongside the test result instead of immediately streaming the message to stdout like `console.log`.


Along the lines of https://nodejs.org/api/console.html#console_console_log_data_args, perhaps:

Print a log message contextually alongside the test result instead of immediately printing it to stdout like console.log.

There are times when you wish to include comments in your tests that will be shown in context of the test result and not streamed to stdout like console.log statements are.

* [x] Switched from `t.comment` to `t.log` * [x] Added mini reporter comments on failure. * [x] Added tests.

Added indentString support. Fixed up readme.

nowells · 2017-07-26T04:18:29Z

The tests that are failing appear to be failing on master as well?

novemberborn

@nowells sorry for the delay. This looks good, though I think you should avoid rewriting the indent-string output. Luckily the indent value can be customized, so with some creative use of its API we can avoid the rewriting.

novemberborn · 2017-07-30T17:06:10Z

lib/reporters/mini.js

+						const logLinesWithLeadingFigure = logLines.replace(
+							/^ {6}/,
+							`    ${colors.information(figures.info)} `
+						);


This might be better as:

const logLines = indentString(colors.log(log), 1, { indent: ` ${colors.information(figures.info)} `, includeEmptyLines: true });

(Though that indent value should probably be a constant, but feel free to redeclare it in the verbose logger.)

See indent-string's API. Including empty lines would be good given the figure prefix.

You'll have to update the indent-string dependency to 3.2.0 since the includeEmptyLines feature is pretty new.

@novemberborn the reason I didn't do that was I only wanted to i symbol to show up once per log line (to indicate where a log message starts)

* this is a message that will have multiple lines, but there only the first line will have the `*` to indicate a log message, and the indent of the other lines is at the indent level of the first log message (after the `*`) * Some other log message

Does that make sense? Or would you rather it be

* this is a message that will have multiple lines, but there only the first line * will have the `*` to indicate a log message, and the indent of the other lines * is at the indent level of the first log message (after the `*`) * Some other log message, but how do I know that.

Oh! No that's fine, I misread the code.

I suppose the replacement string could be a constant, but on the other hand it's clearer if it's near the indentation values. So hey :)

novemberborn · 2017-07-30T18:13:14Z

The tests that are failing appear to be failing on master as well?

I'm hoping #1477 fixes that.

nowells · 2017-07-31T13:47:56Z

thanks for the great feedback and guiding me through this @novemberborn!

sindresorhus · 2017-08-05T16:35:13Z

Thanks for working on this @nowells 🙌

nowells · 2017-08-05T16:40:48Z

@sindresorhus thanks for making such a wonderful testing system!!! I look forward to helping out on other features as well. ❤️

There are times when you wish to include comments in your tests that will be shown in the context of the test result, and not streamed to `stdout` like `console.log` statements are. Fixes #420.

nowells force-pushed the comments branch from fb3570a to 2d98da9 Compare July 13, 2017 16:40

nowells commented Jul 13, 2017

View reviewed changes

nowells force-pushed the comments branch from 2d98da9 to a69aac7 Compare July 14, 2017 12:51

nowells force-pushed the comments branch from 0e5b3d0 to 56344c3 Compare July 20, 2017 16:05

sindresorhus changed the title ~~Add support for test comments.~~ Add support for test logging Jul 25, 2017

novemberborn requested changes Jul 25, 2017

View reviewed changes

nowells added 4 commits July 25, 2017 23:09

Add support for test comments.

5087f15

There are times when you wish to include comments in your tests that will be shown in context of the test result and not streamed to stdout like console.log statements are.

Added PR review comments.

9d4fb37

* [x] Switched from `t.comment` to `t.log` * [x] Added mini reporter comments on failure. * [x] Added tests.

Added README.md docs.

f4946e1

PR feedback.

1bb702b

Added indentString support. Fixed up readme.

nowells force-pushed the comments branch from 02aa7a2 to 1bb702b Compare July 26, 2017 03:10

Merge remote-tracking branch 'origin/master' into comments

69f96e7

novemberborn requested changes Jul 30, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into comments

49bdd02

novemberborn approved these changes Jul 31, 2017

View reviewed changes

novemberborn merged commit 14f7095 into avajs:master Aug 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for test logging #1452

Add support for test logging #1452

nowells commented Jul 13, 2017 •

edited

Loading

nowells Jul 13, 2017 •

edited

Loading

novemberborn commented Jul 20, 2017

nowells commented Jul 20, 2017 •

edited

Loading

nowells commented Jul 20, 2017

sindresorhus commented Jul 25, 2017

sindresorhus commented Jul 25, 2017

nowells commented Jul 25, 2017

novemberborn left a comment

novemberborn Jul 25, 2017

sindresorhus Jul 25, 2017

nowells Jul 26, 2017

novemberborn Jul 25, 2017

novemberborn Jul 25, 2017

nowells Jul 26, 2017

nowells commented Jul 26, 2017

novemberborn left a comment

novemberborn Jul 30, 2017

nowells Jul 31, 2017

novemberborn Jul 31, 2017

novemberborn commented Jul 30, 2017

nowells commented Jul 31, 2017

sindresorhus commented Aug 5, 2017

nowells commented Aug 5, 2017

		if (test.error) {
		return ' ' + colors.error(figures.cross) + ' ' + test.title + ' ' + colors.error(test.error.message);

Add support for test logging #1452

Add support for test logging #1452

Conversation

nowells commented Jul 13, 2017 • edited Loading

Example output

--verbose

--tap

nowells Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

novemberborn commented Jul 20, 2017

nowells commented Jul 20, 2017 • edited Loading

nowells commented Jul 20, 2017

sindresorhus commented Jul 25, 2017

sindresorhus commented Jul 25, 2017

nowells commented Jul 25, 2017

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nowells commented Jul 26, 2017

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn commented Jul 30, 2017

nowells commented Jul 31, 2017

sindresorhus commented Aug 5, 2017

nowells commented Aug 5, 2017

nowells commented Jul 13, 2017 •

edited

Loading

`--verbose`

`--tap`

nowells Jul 13, 2017 •

edited

Loading

nowells commented Jul 20, 2017 •

edited

Loading