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

deps: revert default gtest reporter change #8948

Merged
merged 1 commit into from Oct 23, 2016

Conversation

Projects
None yet
10 participants
@mscdex
Contributor

mscdex commented Oct 6, 2016

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/

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Oct 6, 2016

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

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Oct 6, 2016

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.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Oct 6, 2016

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.

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Oct 6, 2016

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.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Oct 6, 2016

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.

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Oct 6, 2016

It's harder to read and also inconsistent.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Oct 6, 2016

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.

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Oct 6, 2016

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

@bnoordhuis

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.

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Oct 6, 2016

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.

@addaleax

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.

@jasnell

This comment has been minimized.

Member

jasnell commented Oct 6, 2016

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.

@mscdex

This comment has been minimized.

Contributor

mscdex commented Oct 6, 2016

@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?

@jasnell

This comment has been minimized.

Member

jasnell commented Oct 6, 2016

@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.

@mscdex mscdex force-pushed the mscdex:gtest-revert-default-reporter branch 2 times, most recently Oct 6, 2016

@mscdex

This comment has been minimized.

Contributor

mscdex commented Oct 6, 2016

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/

@gibfahn

This comment has been minimized.

Member

gibfahn commented Oct 7, 2016

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).

deps/gtest/src/gtest.cc Outdated
@@ -169,6 +169,9 @@ static const char kUniversalFilter[] = "*";
// The default output file for XML output.
static const char kDefaultOutputFile[] = "test_detail.xml";
// The default result reporter format.
static const char kDefaultResultFormat[] = "pretty";

This comment has been minimized.

@thefourtheye

thefourtheye Oct 7, 2016

Contributor

Unused.

This comment has been minimized.

@mscdex

mscdex Oct 7, 2016

Contributor

Updated.

deps/gtest/src/gtest.cc Outdated
@@ -438,6 +446,17 @@ std::string UnitTestOptions::GetAbsolutePathToOutputFile() {
return result.string();
}
// Functions for processing the gtest_result_format flag.

This comment has been minimized.

@thefourtheye

thefourtheye Oct 7, 2016

Contributor

Isn't this supposed to be singular, Function?

This comment has been minimized.

@mscdex

mscdex Oct 7, 2016

Contributor

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.

@jasnell

jasnell approved these changes Oct 7, 2016

LGTM with the nits raised by @thefourtheye addressed.

@mscdex mscdex force-pushed the mscdex:gtest-revert-default-reporter branch Oct 7, 2016

@mscdex

This comment has been minimized.

Contributor

mscdex commented Oct 10, 2016

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

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Oct 11, 2016

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

@mscdex mscdex force-pushed the mscdex:gtest-revert-default-reporter branch Oct 14, 2016

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Oct 17, 2016

@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.

@jbergstroem

This comment has been minimized.

Member

jbergstroem commented Oct 17, 2016

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

@jbergstroem

This comment has been minimized.

Member

jbergstroem commented Oct 17, 2016

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

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Oct 17, 2016

@jbergstroem Yes, looks good, thanks.

@jbergstroem

This comment has been minimized.

Member

jbergstroem commented Oct 17, 2016

@bnoordhuis cool. Will roll out the change now.

@rvagg rvagg force-pushed the nodejs:master branch 2 times, most recently to 83c7a88 Oct 18, 2016

@mscdex

This comment has been minimized.

Contributor

mscdex commented Oct 23, 2016

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.

@mscdex mscdex force-pushed the mscdex:gtest-revert-default-reporter branch Oct 23, 2016

deps: revert default gtest reporter change
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 mscdex force-pushed the mscdex:gtest-revert-default-reporter branch to 6e5389e Oct 23, 2016

@mscdex mscdex closed this Oct 23, 2016

@mscdex mscdex deleted the mscdex:gtest-revert-default-reporter branch Oct 23, 2016

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

jasnell added a commit that referenced this pull request Oct 24, 2016

deps: revert default gtest reporter change
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>

@MylesBorins MylesBorins added this to the 4.7.0 milestone Nov 14, 2016

MylesBorins added a commit that referenced this pull request Nov 14, 2016

deps: revert default gtest reporter change
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 a commit to mscdex/io.js that referenced this pull request Nov 18, 2016

deps: revert default gtest reporter change
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>

@mscdex mscdex referenced this pull request Nov 18, 2016

Closed

backport: cctest TAP output changes #9682

2 of 2 tasks complete

MylesBorins added a commit that referenced this pull request Nov 22, 2016

deps: revert default gtest reporter change
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>

@MylesBorins MylesBorins removed this from the 4.7.0 milestone Nov 22, 2016

This was referenced Nov 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment