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

Record reference test stdout and stderr #7917

Merged
merged 1 commit into from
Oct 8, 2015

Conversation

mrobinson
Copy link
Member

When reference tests crash it is difficult to debug the issue, because
the stacktrace and any other output is discarded by the test harness.
Instead of discarding it, write the output to text files in the test
results directory which will make it easier to debug crashing tests.

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 7, 2015
When reference tests crash it is difficult to debug the issue, because
the stacktrace and any other output is discarded by the test harness.
Instead of discarding it, write the output to text files in the test
results directory which will make it easier to debug crashing tests.
@jdm
Copy link
Member

jdm commented Oct 7, 2015

Any particular reason not to use Stdio::inherit() and Stderr::inherit(), to get the output into the standard logging on the build machines?

@jdm
Copy link
Member

jdm commented Oct 7, 2015

Alterntively, if all the reftests were translated to WPT reftests than this issue would be moot :)

@mrobinson
Copy link
Member Author

@jdm I can definitely explore that, but I originally did not do that because I thought the output would be interlaced confusingly with the output from the test harness itself.

@mrobinson
Copy link
Member Author

@jdm Stderr::inherit(), does seem to mix the backtraces with the output of the test harness, which makes the output quite a bit more confusing. I think that the output file is a better option in this case.

I also agree that it's a good idea to migrate the ref-tests to be W3C style tests, but that is a much more complicated task, so I think this is a decent stopgap solution. This change was very helpful for debugging a recent issue that only appeared during reftesting.

@jdm
Copy link
Member

jdm commented Oct 8, 2015

Ok. I guess my only remaining concern is that this is not Windows friendly, and it would be nice to fix that.

@jdm
Copy link
Member

jdm commented Oct 8, 2015

I also have vague concerns about the temp directories becoming crowded on the build machines? Not sure how much of a problem that is; any thoughts @larsbergstrom?

@mrobinson
Copy link
Member Author

@jdm Could you elaborate a bit on the Windows concerns? Is it that the path names are too long?

I'm not sure if it is feasible, but moving the results from a temporary directory to a checkout specific directory would make it possible to clear the results between runs and allow running tests for two checkouts in parallel.

@jdm
Copy link
Member

jdm commented Oct 8, 2015

I was assuming that /tmp wouldn't work, but maybe if we rely on msys out of the gate that's not a problem.

@mrobinson
Copy link
Member Author

@jdm or @larsbergstrom Do you have a sense if moving the results to the build directory would be useful and possible? To me it seems like a good option because it is simpler to clean them and they will not clash between checkouts.

@larsbergstrom
Copy link
Contributor

@bors-servo r+

I think this will definitely help with debugging in the short term. In the longer term, @Ms2ger is planning to rewrite these tests as WPT tests and remove the harness entirely, so I'm fine with doing anything that eases immediate pain, so long as it's not a huge investment since this tool is on its way out.

@bors-servo
Copy link
Contributor

📌 Commit 3cf4acd has been approved by larsbergstrom

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 8, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 3cf4acd with merge d15ea83...

bors-servo pushed a commit that referenced this pull request Oct 8, 2015
Record reference test stdout and stderr

When reference tests crash it is difficult to debug the issue, because
the stacktrace and any other output is discarded by the test harness.
Instead of discarding it, write the output to text files in the test
results directory which will make it easier to debug crashing tests.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7917)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

@bors-servo bors-servo merged commit 3cf4acd into servo:master Oct 8, 2015
@mrobinson mrobinson deleted the test-stderr branch October 8, 2015 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants