Bug 98551 – TestResultsServer does not display sync_integration_tests results

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Richard Larocque]

Created attachment 167374 [details]
Trivial patch to fix the bug

This bug is about the server that resides in Tools/TestResultsServer, an instance of which can be found at test-results.appspot.com.

Some of the builder infrastructure for chromium has been shuffled around lately.  The sync_integration_tests are now run on dedicated builders, none of which include the word "Tests" in their title.  This means that all builders that would be able to provide information about sync_integration_tests are filtered out by isChromiumDepsGTestRunner() in builder.js.  If you click on the URL mentioned in the bug (http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40DEPS%20-%20chromium.org&testType=sync_integration_tests), you'll see that there are no results for sync_integration_tests.  However, http://test-results.appspot.com/testfile?testtype=sync_integration_tests shows that there are plenty of tests available.

I've attached a trivial patch that makes isChromiumDepsGTestRunner() no longer filter out the sync builders.  I have verified locally that this allows the tool to return results for sync_integration_tests when the 'group' is set to @DEPS.  I'm not too familiar with this code, so this may not be the best solution.  I hope it's a useful starting point.

[Ojan Vafai]

Your patch needs a ChangeLog description. Normally it would need a test too, but I think this change is basically covered by existing tests. You should include a line about why you don't need a test in the ChangeLog description.

See the section on ChangeLogs at http://www.webkit.org/coding/contributing.html.

I recommend using "Tools/Scripts/webkit-patch upload --request-commit" to upload the patch as that will autogenerate the ChangeLog boilerplate for you and properly mark this patch as needing code review + commit queue once it's been approved.

What I told you in email about not needing to change the groups for this test suite was technically wrong since I see now that they only run on the main Chromium waterfall, but I think it's fine to leave the patch as is (less code complexity than adding a new group). If people try to pick one of the other groups, they'll just get an empty dashboard page.

[Richard Larocque]

Created attachment 167388 [details]
Patch

[WebKit Review Bot]

Attachment 167388 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/TestResultServer..." exit_code: 1
Tools/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Tools/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Tools/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Tools/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ojan Vafai]

Comment on attachment 167388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167388&action=review

>> Tools/ChangeLog:12
>> +	updated to match the new behaviour.
> 
> Line contains tab character.  [whitespace/tab] [5]

As annoying as it is, these need to be 8-space indented.

[Richard Larocque]

Created attachment 167390 [details]
Patch

[Ojan Vafai]

Comment on attachment 167390 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 167390 [details]
Patch

Clearing flags on attachment: 167390

Committed r130562: <http://trac.webkit.org/changeset/130562>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.