tools: fix regression in doctool #6680

MylesBorins · 2016-05-10T18:59:17Z

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

tools

Description of change

101dd1e introduced a regression in the doctool. This commit reverts
the changes that were made to the function signature of the various
doctool functions while maintaining support for passing in specific
node versions.

MylesBorins · 2016-05-10T19:00:11Z

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

/cc @nodejs/documentation

MylesBorins · 2016-05-10T20:30:29Z

The changes regressed #5873

Fishrock123 · 2016-05-10T20:35:15Z

LGTM

eljefedelrodeodeljefe · 2016-05-10T20:47:14Z

LGTM

jmm · 2016-05-10T21:13:08Z

@thealphanerd I'd like to understand what went wrong here. Merging #3888 broke something other than the callsite signature in the test file that was merged at the end of April?

How could I go about testing this to see how it fails (other than that mismatched callsite signature in the test file)?

MylesBorins · 2016-05-10T21:14:24Z

CI failures are unrelated. I've gone ahead and landed this on master to stop us from breaking CI.

@jmm ~~it actually broke a unit test. If you git checkout 101dd1e and run make test you'll see the failure.~~

~~The signature for html needed to be updated in the test, and then the output was no longer correct.~~

MylesBorins · 2016-05-10T21:21:59Z

Womp... didn't do good reading comprehension on your response. If you fix the signature in the test then two of the tests were still failing as html was outputting the wrong information. The first test case passed, but the test case to guard against the regression in #5873 broke. reverting the signature change fixed the tests.

Most likely there was an argument that didn't have opt in front of it. Reverting the signature with an optional nodeVersion argument allowed the tool to support the new functionality and pass the tests... so I just settled on it.

I'd be interested to find out what caused the bug if you dig into it

jmm · 2016-05-10T21:23:10Z

@jmm it actually broke a unit test. If you git checkout 101dd1e and run make test you'll see the failure.

I saw in the CI report that there was a failure in test/doctool/test-doctool-html.js so I took a look and saw that the signature didn't match the updates from my PR and ran it directly (with my existing Node -- node test/doctool/test-doctool-html.js) and saw it fail.

and then the output was no longer correct.

Really? When I check out that commit and update the calls to the updated signature from my PR I get no failure. That's with my existing Node though. I'm running the full make test / Node build right now to check it out -- I don't understand yet how updating those signatures would cause incorrect output once the callsites are updated in the test file.

MylesBorins · 2016-05-10T21:34:20Z

@jmm I just tried retracing the steps again myself and was able to get the tests to pass as expected

I'm not sure what was going on with my environment to get it to fail before. Odd. I was just as confused when this worked, but ran with it. I moved a little quicker than normal with this due to the tests being broken on master.

I see no reason why you couldn't submit another PR that does some refactoring on the doc-tool if you would like to make a change to what is currently on master.

jmm · 2016-05-10T22:50:24Z

@thealphanerd Ok, cool.

I moved a little quicker than normal with this due to the tests being broken on master.

Totally understandable. Sorry the test was broken at all.

I see no reason why you couldn't submit another PR that does some refactoring on the doc-tool if you would like to make a change to what is currently on master.

Sounds good. I still think converting those functions to take options hashes makes sense, so will do.

These signatures were originally converted to opts hashes in nodejs#3888. That change was misinterpreted as the intrinsic cause of a test failure and reverted in nodejs#6680.

101dd1e introduced a regression in the doctool. This commit reverts the changes that were made to the function signature of the various doctool functions while maintaining support for passing in specific node versions. Refs: 101dd1e PR-URL: #6680 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>