tools: expose skip output to test runner by jbergstroem · Pull Request #2130 · nodejs/node

jbergstroem · 2015-07-08T00:44:06Z

In the TAP protocol, skips are flagged as ok. Expose more information so we can understand if the test was skipped or not. Refs #2109.

(see http://testanything.org/tap-specification.html -> directives -> skipping tests)

/R=@nodejs/build, @bnoordhuis (keeps my python on track)

In the TAP protocol, skips are flagged as ok. Expose more information so we can understand if the test was skipped or not.

rmg · 2015-07-08T00:50:58Z

This should probably be a regex if there's any concern about actual TAP compliance. The Skipped only needs to match skip, and it shouldn't be case sensitive.

jbergstroem · 2015-07-08T03:12:35Z

Yeah. String matching is questionable but since we're at least establishing a standard for this, I chose to go with the cheaper operation.

The "right thing" to do here is creating a function that formats into tap input in common.js we can call from all the places. I guess the even more correct thing would be moving into a test runner that does this for us which also would give us the benefit of not having to spawn io.js on every request.

Anyway, if more people agree that I should go regex I'll make it so.

rmg · 2015-07-08T06:39:26Z

I wouldn't worry about performance until it's a problem. An anchored regex is normally just as fast or faster than a string comparison because it gets compiled to a dfa (though that generally only matters for longer strings). The case insensitively would be the biggest cost regardless of the method used.

jbergstroem · 2015-07-08T07:01:04Z

⬆️ fixed

`re` only matches on the first line unless told to do multiline.

thefourtheye · 2015-07-08T07:20:03Z

It is better to use raw strings for RegEx. Also, we can precompile and store the RegEx with re.compile

orangemocha · 2015-07-08T10:59:49Z

Just a question, since I am about to touch this code too, where is the "# SKIP" output generated? Is it logged to stdout by the tests themselves?
There is also support in the test runner to do this with status files.

@orangemocha PTAL at #2109

Yes, the tests print '0..1 # Skipped: <reason>'.

Got it, thanks!

jbergstroem · 2015-07-08T22:48:36Z

Updated based on all feedback disregarding code style -- will update if we don't care about current conventions.

rmg · 2015-07-08T23:06:56Z

The regex appears to be fed a multiline blob of text.. does anything need to be done about only capturing the the end of the line?

I would expect not but it has been a while since I used Python so I just wanted to point it out.

jbergstroem · 2015-07-08T23:28:37Z

@rmg as far as I know, re will only capture the first line unless you pass the re.MULTILINE flag.

jbergstroem · 2015-07-08T23:32:25Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/127/

jbergstroem · 2015-07-09T00:21:26Z

Here's two skip's in output: https://jenkins-iojs.nodesource.com/job/iojs+pr+win/nodes=win2008r2/113/console (test 133, 134)

~~Doesn't look like the tap plugin we run for jenkins picks them up though:~~https://jenkins-iojs.nodesource.com/job/iojs+pr+win/nodes=win2008r2/113/tapTestReport/

Edit: Works as intended, I was just a bit early to call it.

thefourtheye · 2015-07-09T10:42:47Z

Hmmm, its better not to search twice. Why not use @bnoordhuis's suggestion #2130 (comment)?

Because code conventions. I'll make it happen.

bnoordhuis · 2015-07-09T10:57:52Z

re will only capture the first line unless you pass the re.MULTILINE flag.

Depends on whether you use re.match() or re.search(). re.match() matches from the start of the string, re.search() matches anywhere in the string.

re.MULTILINE only affects anchors, you may have been thinking of re.DOTALL. It makes . match newlines.

jbergstroem · 2015-07-13T02:53:19Z

^ PTAL

thefourtheye · 2015-07-13T04:24:57Z

The original version had self.store_unexpected_output check as well. It is not there now. Is it okay?

jbergstroem · 2015-07-13T05:20:19Z

@thefourtheye yeah, represents whether the test suite should catch body output or not. I don't think too many people would turn that off. Reckon the regex is cheap enough or do we want to squeeze that extra cpu cycle?

thefourtheye · 2015-07-13T06:34:00Z

@jbergstroem Cool then. The RegEx looks fine and overall the code is readable to me. LGTM 👍

jbergstroem · 2015-07-13T22:59:06Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/149/

Edit: Skip here.

jbergstroem · 2015-07-16T02:11:07Z

@bnoordhuis went ahead and did a similar suggestion to yours. Looking better?

Fishrock123 · 2015-07-28T17:43:35Z

Is this good to land then?

thefourtheye · 2015-07-28T17:50:08Z

LGTM 👍, if the CI run is green.

bnoordhuis · 2015-07-28T23:59:52Z

Style nit: if it doesn't fit on a single line, maybe write it as:
logger.info( 'ok %i - %s # skip %s'% (self._done, command, skip.group(1)))

bnoordhuis · 2015-07-29T00:00:05Z

LGTM with nit and sorry for the delay.

In the TAP protocol, skips are flagged as ok. Expose more information so we can understand if the test was skipped or not. PR-URL: #2130 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jbergstroem · 2015-07-30T07:53:00Z

Merged in 3cbb587. Thanks for the review.

tools: expose skip output to test runner
ea2fa68
In the TAP protocol, skips are flagged as ok. Expose more information so we can understand if the test was skipped or not.

jbergstroem added the test Issues and PRs related to the tests. label Jul 8, 2015

rmg reviewed Jul 8, 2015
View reviewed changes

fix: use regex instead of string matching
b673537

fix: avoid calling strip
de4e4d0
`re` only matches on the first line unless told to do multiline.

thefourtheye reviewed Jul 8, 2015
View reviewed changes

fix: use raw text and compile regex
3014572

orangemocha reviewed Jul 8, 2015
View reviewed changes

jbergstroem added 2 commits July 9, 2015 08:45

fix: use 'official' regex and rename var
4d6b0b5

fix: switch to re.search
9cd5da6

thefourtheye reviewed Jul 9, 2015
View reviewed changes

fix: simplify logic by avoiding classes
3749544

jbergstroem force-pushed the feature/expose-skipped-tests branch from 6725cba to 3749544Compare July 13, 2015 02:38

brendanashworth added the tools Issues and PRs related to the tools directory. label Jul 19, 2015

thefourtheye mentioned this pull request Jul 20, 2015
test: formatting skip messages for TAP parsing #2109
Closed

indutny force-pushed the master branch from bffb204 to eb35968Compare July 22, 2015 21:21

bnoordhuis reviewed Jul 28, 2015
View reviewed changes

jbergstroem closed this Jul 30, 2015

rvagg mentioned this pull request Aug 4, 2015
Release proposal: v3.0.0 #2299
Merged

Uh oh!

Uh oh!

Conversation

jbergstroem commented Jul 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbergstroem commented Jul 8, 2015

Uh oh!

rmg commented Jul 8, 2015

Uh oh!

jbergstroem commented Jul 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbergstroem commented Jul 8, 2015

Uh oh!

rmg commented Jul 8, 2015

Uh oh!

jbergstroem commented Jul 8, 2015

Uh oh!

jbergstroem commented Jul 8, 2015

Uh oh!

jbergstroem commented Jul 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jul 9, 2015

Uh oh!

jbergstroem commented Jul 13, 2015

Uh oh!

thefourtheye commented Jul 13, 2015

Uh oh!

jbergstroem commented Jul 13, 2015

Uh oh!

thefourtheye commented Jul 13, 2015

Uh oh!

jbergstroem commented Jul 13, 2015

Uh oh!

jbergstroem commented Jul 16, 2015

Uh oh!

Fishrock123 commented Jul 28, 2015

Uh oh!

thefourtheye commented Jul 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jul 29, 2015

Uh oh!

jbergstroem commented Jul 30, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants