deps: revert default gtest reporter change by mscdex · Pull Request #8948 · nodejs/node · GitHub

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

gtest

Description of change

The default gtest reporter changed in c56ae16 from a pretty printing reporter to TAP. This caused TAP output to be displayed when running make test locally (outside of CI). This commit reverts that particular change.

/cc @bnoordhuis @jasnell

CI: https://ci.nodejs.org/job/node-test-pull-request/4405/

I made that change intentionally. Why would you want a different output format locally?

Can we leave the tap output for make test-ci and use the pretty one locally? There's not really a reason to have tap output in the local tests.

I think consistency with the CI is a pretty good reason. Libuv does the same thing; it makes visually comparing the output of local and remote runs a lot easier.

If you want CI output, just run make test-ci -- we aren't the same as Libuv. Do you want to change the JS test output too? If not, this should be reverted for local tests.

You asked for a reason and I gave you one. You are welcome to disagree but at least try to come up with a counterargument.

It's harder to read and also inconsistent.

Readability is subjective but I agree it's inconsistent. Then again, so is the normal gtest output vis-a-vis the JS test runner and the linters. It doesn't bother me personally but I'm okay with switching to TAP wholesale if you think consistency is important.

I'm not ok with that as it would mean scrolling through a few thousand lines of terminal text to find an error.

I'll -1 as a counterweight to your +1 because I don't think your readability and consistency arguments are convincing. Besides, I like the new format.

If you really want it, we can make make test-tap. It is far easier to do that than update all the docs references to point to some new make test-pretty for everyone new coming here.

Yeah, I have to agree with @Fishrock123 and @mscdex here… being greeted by a tap Wall of Text doesn’t seem very helpful to me.

So, +1 to the revert here, although I realize that this is purely subjective from me, too.

fwiw, I'm partial to the new formatting but will go with whatever the majority opinion is here. Perhaps rather than simply revert, this PR could add a test-tap target for those of us who prefer it.

@jasnell Are you saying test-tap would output TAP just for cctest but use progress indicators/"pretty print" for all other tests (like it is before this PR), or?

@mscdex... For now, test-tap would use the current behavior (before this revert). Eventually I'd like to see test-tap do the full output as TAP.

Ok, I've added test-tap that uses a new flag to configure the default printed result format.

CI again: https://ci.nodejs.org/job/node-test-pull-request/4410/

It'd be really useful to have both TAP and pretty formats available for running tests, I frequently find myself using both.

Is it possible to have the gtest pretty format match the tools/test.py pretty format? Ideally you'd just have the one line for everything (assuming no failures).

Unused.

Updated.

Isn't this supposed to be singular, Function?

I kept it that way for consistency in case more are added in the future. I see it as more of a list heading than a comment for the one function.

LGTM with the nits raised by @thefourtheye addressed.

@bnoordhuis Is this acceptable to you (make test-tap)?

If the choice is between no TAP output locally or carrying more patches on top of gtest, I'd rather go with the former.

@jbergstroem Did you ever make the change to make the CI pick up cctest.tap? Looking at e.g. https://ci.nodejs.org/job/node-test-commit-linux/5713/nodes=centos5-32/console I get the impression it doesn't currently.

@bnoordhuis no, let me fix that now. Just very tedious working with jenkins jobs when its slow :'(

@bnoordhuis just did a test run with aix61. Looks good -- do you agree? https://ci.nodejs.org/job/node-test-commit-aix/1508/

@jbergstroem Yes, looks good, thanks.

@bnoordhuis cool. Will roll out the change now.

CI once more before landing: https://ci.nodejs.org/job/node-test-pull-request/4626/

EDIT: CI is green except for a few flaky tests on freebsd and pi1.

PR-URL: nodejs#8948 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: #8948 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: nodejs#8948 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: #8948 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. tools Issues and PRs related to the tools directory. gtest labels Oct 6, 2016

nodejs-github-bot added the gtest label Oct 6, 2016

Fishrock123 approved these changes Oct 6, 2016

View reviewed changes

bnoordhuis requested changes Oct 6, 2016

View reviewed changes

addaleax approved these changes Oct 6, 2016

View reviewed changes

mscdex force-pushed the gtest-revert-default-reporter branch 2 times, most recently from 03824c8 to a205a9e Compare October 6, 2016 21:24

thefourtheye reviewed Oct 7, 2016

View reviewed changes

jasnell approved these changes Oct 7, 2016

View reviewed changes

mscdex force-pushed the gtest-revert-default-reporter branch from a205a9e to f9ad5da Compare October 7, 2016 18:12

mscdex force-pushed the gtest-revert-default-reporter branch from f9ad5da to cebefd4 Compare October 14, 2016 23:27

bnoordhuis approved these changes Oct 17, 2016

View reviewed changes

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

mscdex force-pushed the gtest-revert-default-reporter branch from d12ed28 to cd3b53e Compare October 23, 2016 08:32

mscdex force-pushed the gtest-revert-default-reporter branch from cd3b53e to 6e5389e Compare October 23, 2016 08:34

mscdex closed this Oct 23, 2016

mscdex deleted the gtest-revert-default-reporter branch October 23, 2016 08:35

mscdex merged commit 6e5389e into nodejs:master Oct 23, 2016

mscdex mentioned this pull request Oct 24, 2016

build: run cctests as part of test-ci target #8034

Merged

MylesBorins added the lts-watch-v4.x label Oct 24, 2016

MylesBorins added the land-on-v6.x label Nov 14, 2016

MylesBorins added this to the 4.7.0 milestone Nov 14, 2016

mscdex mentioned this pull request Nov 18, 2016

backport: cctest TAP output changes #9682

Closed

2 tasks

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Nov 18, 2016

MylesBorins removed this from the 4.7.0 milestone Nov 22, 2016

This was referenced Nov 22, 2016

v6.9.2 proposal #9735

Merged

v4.7.0 proposal #9736

Merged

deps: revert default gtest reporter change by mscdex · Pull Request #8948 · nodejs/node · GitHub

Uh oh!

Uh oh!

deps: revert default gtest reporter change - #8948

deps: revert default gtest reporter change#8948mscdex merged 1 commit intonodejs:masternodejs/node:masterfrom mscdex:gtest-revert-default-reportermscdex/io.js:gtest-revert-default-reporterCopy head branch name to clipboard

Conversation

mscdex commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

bnoordhuis commented Oct 6, 2016

Uh oh!

Fishrock123 commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Oct 6, 2016

Uh oh!

Fishrock123 commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Oct 6, 2016

Uh oh!

Fishrock123 commented Oct 6, 2016

Uh oh!

bnoordhuis commented Oct 6, 2016

Uh oh!

Fishrock123 commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 6, 2016

Uh oh!

mscdex commented Oct 6, 2016

Uh oh!

jasnell commented Oct 6, 2016

Uh oh!

mscdex commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Oct 7, 2016

Uh oh!

thefourtheye Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented Oct 10, 2016

Uh oh!

bnoordhuis commented Oct 11, 2016

Uh oh!

bnoordhuis commented Oct 17, 2016

Uh oh!

jbergstroem commented Oct 17, 2016

Uh oh!

jbergstroem commented Oct 17, 2016

Uh oh!

bnoordhuis commented Oct 17, 2016

Uh oh!

jbergstroem commented Oct 17, 2016

Uh oh!

deps: revert default gtest reporter change#8948
mscdex merged 1 commit into
nodejs:masterfrom
mscdex:gtest-revert-default-reporter

mscdex commented Oct 6, 2016 •
edited
Loading

Fishrock123 commented Oct 6, 2016 •
edited
Loading

Fishrock123 commented Oct 6, 2016 •
edited
Loading

Fishrock123 commented Oct 6, 2016 •
edited
Loading

Fishrock123 commented Oct 6, 2016 •
edited
Loading

mscdex commented Oct 6, 2016 •
edited
Loading

mscdex commented Oct 23, 2016 •
edited
Loading