Skip to content
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

tools: expose skip output to test runner #2130

Closed

Conversation

jbergstroem
Copy link
Member

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.
@jbergstroem jbergstroem added the test Issues and PRs related to the tests. label Jul 8, 2015
@@ -471,6 +474,10 @@ def UnexpectedOutput(self):
outcome = PASS
return not outcome in self.test.outcomes

def HasSkipped(self):
s = '1..0 # Skipped:'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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
Copy link
Contributor

rmg commented Jul 8, 2015

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
Copy link
Member Author

⬆️ fixed

`re` only matches on the first line unless told to do multiline.
@@ -255,6 +255,10 @@ def HasRun(self, output):
logger.info('#' + l)
for l in output.output.stdout.splitlines():
logger.info('#' + l)
elif output.HasSkipped():
skip = re.findall('# SKIP\S+ (.+)', output.output.stdout,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -471,6 +475,10 @@ def UnexpectedOutput(self):
outcome = PASS
return not outcome in self.test.outcomes

def HasSkipped(self):
skip = is_skipped.search(self.output.stdout)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks!

@jbergstroem
Copy link
Member Author

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

@rmg
Copy link
Contributor

rmg commented Jul 8, 2015

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
Copy link
Member Author

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

@jbergstroem
Copy link
Member Author

@jbergstroem
Copy link
Member Author

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.

@@ -255,6 +256,9 @@ def HasRun(self, output):
logger.info('#' + l)
for l in output.output.stdout.splitlines():
logger.info('#' + l)
elif output.HasSkipped():
skip = skip_regex.search(output.output.stdout).group(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because code conventions. I'll make it happen.

@bnoordhuis
Copy link
Member

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
Copy link
Member Author

^ PTAL

@thefourtheye
Copy link
Contributor

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

@jbergstroem
Copy link
Member Author

@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
Copy link
Contributor

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

@jbergstroem
Copy link
Member Author

@jbergstroem
Copy link
Member Author

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

@brendanashworth brendanashworth added the tools Issues and PRs related to the tools directory. label Jul 19, 2015
@Fishrock123
Copy link
Contributor

Is this good to land then?

@thefourtheye
Copy link
Contributor

LGTM 👍, if the CI run is green.

skip = skip_regex.search(output.output.stdout)
if skip:
logger.info('ok %i - %s # skip %s' %
(self._done, command, skip.group(1)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

LGTM with nit and sorry for the delay.

jbergstroem added a commit that referenced this pull request Jul 30, 2015
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
Copy link
Member Author

Merged in 3cbb587. Thanks for the review.

@rvagg rvagg mentioned this pull request Aug 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants