WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
98422
[TestResultServer] Add support for non-Chromium TestExpectations files
https://bugs.webkit.org/show_bug.cgi?id=98422
Summary
[TestResultServer] Add support for non-Chromium TestExpectations files
Zan Dobersek
Reported
2012-10-04 10:19:21 PDT
[TestResultServer] Add support for non-Chromium TestExpectations files
Attachments
Patch
(43.61 KB, patch)
2012-10-04 11:01 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(43.54 KB, patch)
2012-10-11 04:08 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(47.28 KB, patch)
2012-10-12 05:33 PDT
,
Zan Dobersek
ojan
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2012-10-04 11:01:22 PDT
Created
attachment 167131
[details]
Patch
Zan Dobersek
Comment 2
2012-10-04 11:25:27 PDT
(In reply to
comment #1
)
> Created an attachment (id=167131) [details] > Patch
The ChangeLog is quite detailed, but feel free (or obliged :>) to raise any other issues.
Ojan Vafai
Comment 3
2012-10-10 11:30:05 PDT
Comment on
attachment 167131
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=167131&action=review
Thanks for working on this! This stuff it crazy complicated. Overall, my comments are mostly about style. The only correctness concern I have is the early "return false" in processExpectations. I've long wished that we could avoid duplicating all the python logic for this, but this patch a fine interim step to keep things working until then. What I had in mind is that we'd have some python code that would generate JSON representing the expectations/modifiers for each test on each platform. Something like: { "fast": { "ChromiumLion": { buildTypes: [...], bugs: [...], expectations: [...] } "AppleLion": {...}, "forms": { "ChromiumLion": { buildTypes: [...], bugs: [...], expectations: [...] } "AppleMountainLion": {...}, } } } Once we have the python code generate this global expectations json file, then the flakiness dashboard just needs to walk this trie and doesn't need to duplicate (and more importantly, keep up to date with) the python logic. As to this patch, at a high-level, I'm trying to incrementally move this code away from being a big group of global functions into being more object oriented. One place you could maybe do so here is to make the trie a proper class. function TestTrie() TestTrie.prototype.addTest(test) // instead of addTestToTrie TestTrie.prototype.forEach(callback) // instead of allTestsTrieTraversal
> Tools/TestResultServer/static-dashboards/dashboard_base.js:566 > + (function requestExpectationsFile() {
Why not request all the expectations files in parallel? Won't that improve dashboard load time? I think it would also simplify the code a bit.
> Tools/TestResultServer/static-dashboards/dashboard_base.js:569 > + logTime('requestExpectationsFiles: ', start);
Nit: here and elsewhere. I've been trying to cut back on the amount of stuff we always spew to the console. Logging these things it not much more useful than what you can get automatically with modern dev tools (e.g. network panel + profiler).
> Tools/TestResultServer/static-dashboards/dashboard_base.js:622 > + g_waitingOnExpectations = isLayoutTestResults() && !isTreeMap();
lol. whoops!
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:283 > +function platformsTreeTraversal(callback)
I find the non-verbness of this name confusing. How about traversePlatformsTree?
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:287 > + for (var i = 0; i < platformsList.length; i++) {
Nit: How about using platformsList.forEach(...)?
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:332 > +function determineWKPlatform(builderName, basePlatform) {
Nit: curly on new line
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:439 > +function allTestsTrieTraversal(callback, startingTriePath) {
Ditto: How about traverseAllTestsTrie?
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:663 > + for (var i = 1; i < platformsList.length; i++)
Nit: use forEach
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:692 > + for (var i = 0; i < subPlatformsList.length; i++)
Nit: use forEach
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:861 > + if (platform.platformModifierUnions) { > + processExpectationsForPlatform(platformName, g_expectationsByPlatform[platformName]); > + return false; > + }
I'm confused by this. For Chromium, we don't look at subplatforms? Shouldn't we return true here?
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:866 > + if (fallbackPlatform in g_expectationsByPlatform)
Isn't it an error if fallbackPlatform isn't in g_expectationsByPlatform? I think it makes sense to fail gracefully, but maybe we should console.error?
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:871 > + if (platformName in g_expectationsByPlatform)
ditto
Zan Dobersek
Comment 4
2012-10-11 03:01:30 PDT
Comment on
attachment 167131
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=167131&action=review
>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:861 >> + } > > I'm confused by this. For Chromium, we don't look at subplatforms? Shouldn't we return true here?
Chromium has a TestExpectations file that's (in the current PLATFORMS tree structure) tied to the base CHROMIUM platform, with platform modifier unions and subplatform modifiers used in it extensively. Because of that these expectations are processed for the base CHROMIUM platform but not for its subplatforms (hence the returning of false value). Any possible platform modifiers or unions are then appropriately processed in addTestToAllExpectationsForPlatform.
>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:866 >> + if (fallbackPlatform in g_expectationsByPlatform) > > Isn't it an error if fallbackPlatform isn't in g_expectationsByPlatform? I think it makes sense to fail gracefully, but maybe we should console.error?
That makes for a lot of console output when running unit tests, but I can put the console.error call in if desired.
Zan Dobersek
Comment 5
2012-10-11 04:08:34 PDT
Created
attachment 168189
[details]
Patch
Zan Dobersek
Comment 6
2012-10-11 04:11:29 PDT
(In reply to
comment #5
)
> Created an attachment (id=168189) [details] > Patch
Adds the TestTrie class, requests TestExpectations files in parallel, fixes the nits. The raised issues left open were responded to in
comment #4
.
Ojan Vafai
Comment 7
2012-10-11 16:56:32 PDT
Comment on
attachment 168189
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=168189&action=review
This looks great. Just a few more tests if you don't mind.
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:425 > +var TestTrie = function()
How about taking builders as an argument to the constructor instead? Then you can easily test this by passing in a more manageable object and pass in g_builders in the actual code. Mind adding some tests for this? Shouldn't be too hard to get good test coverage of all the code paths.
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:885 > + logTime('processExpectations: ', start);
Nit: please remove this and the start variable.
Ojan Vafai
Comment 8
2012-10-11 17:12:30 PDT
Comment on
attachment 167131
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=167131&action=review
>>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:861 >>> + } >> >> I'm confused by this. For Chromium, we don't look at subplatforms? Shouldn't we return true here? > > Chromium has a TestExpectations file that's (in the current PLATFORMS tree structure) tied to the base CHROMIUM platform, with platform modifier unions and subplatform modifiers used in it extensively. Because of that these expectations are processed for the base CHROMIUM platform but not for its subplatforms (hence the returning of false value). Any possible platform modifiers or unions are then appropriately processed in addTestToAllExpectationsForPlatform.
ANDROID should have CHROMIUM_ANDROID in its fallbackPlatforms list. I think that might be the confusion. Maybe Chromium platforms shouldn't have 'CHROMIUM' in the fallback list and should just have an empty fallback list except for CHROMIUM_ANDROID, and that would need to go through the fallbackPlatforms codepath as well?
>>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:866 >>> + if (fallbackPlatform in g_expectationsByPlatform) >> >> Isn't it an error if fallbackPlatform isn't in g_expectationsByPlatform? I think it makes sense to fail gracefully, but maybe we should console.error? > > That makes for a lot of console output when running unit tests, but I can put the console.error call in if desired.
I see. Ideally we'd change processExpectations to take g_expectationsByPlatform, then the tests can pass in a dummy object and we can make sure to test every codepath. That's too much for this patch though. Maybe add a FIXME for now?
Zan Dobersek
Comment 9
2012-10-12 02:05:52 PDT
(In reply to
comment #8
)
> (From update of
attachment 167131
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=167131&action=review
> > >>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:861 > >>> + } > >> > >> I'm confused by this. For Chromium, we don't look at subplatforms? Shouldn't we return true here? > > > > Chromium has a TestExpectations file that's (in the current PLATFORMS tree structure) tied to the base CHROMIUM platform, with platform modifier unions and subplatform modifiers used in it extensively. Because of that these expectations are processed for the base CHROMIUM platform but not for its subplatforms (hence the returning of false value). Any possible platform modifiers or unions are then appropriately processed in addTestToAllExpectationsForPlatform. > > ANDROID should have CHROMIUM_ANDROID in its fallbackPlatforms list. I think that might be the confusion. > > Maybe Chromium platforms shouldn't have 'CHROMIUM' in the fallback list and should just have an empty fallback list except for CHROMIUM_ANDROID, and that would need to go through the fallbackPlatforms codepath as well? >
Ah, Chromium Android has its own TestExpectations file. That makes the current approach of not traversing into subplatforms of a platform with modifier unions incorrect. I'll fix that.
> >>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:866 > >>> + if (fallbackPlatform in g_expectationsByPlatform) > >> > >> Isn't it an error if fallbackPlatform isn't in g_expectationsByPlatform? I think it makes sense to fail gracefully, but maybe we should console.error? > > > > That makes for a lot of console output when running unit tests, but I can put the console.error call in if desired. > > I see. Ideally we'd change processExpectations to take g_expectationsByPlatform, then the tests can pass in a dummy object and we can make sure to test every codepath. > > That's too much for this patch though. Maybe add a FIXME for now?
OK.
Zan Dobersek
Comment 10
2012-10-12 05:33:58 PDT
Created
attachment 168403
[details]
Patch
Zan Dobersek
Comment 11
2012-10-12 05:43:46 PDT
(In reply to
comment #10
)
> Created an attachment (id=168403) [details] > Patch
Adds a test for TestTrie, loads the Chromium Android test expectations file, platforms tree traversal now executes callback only on leaf nodes, meaning that when processing expectations for a specific platform (in processExpectationsForPlatform) the fallback platforms are checked for any platform modifier unions. The expectations' modifiers are then searched to see if any represent such unions - if so, the expectation is only processed for that platform if the modifier in the expectation represents an union of which the platform is a member.
Ojan Vafai
Comment 12
2012-10-12 13:57:23 PDT
Comment on
attachment 168403
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=168403&action=review
Thanks for fixing this and being so responsive to the code review comments!
> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:52 > + 'ANDROID': { fallbackPlatforms: ['CHROMIUM'], expectationsDirectory: 'chromium-android' }
Shouldn't this be ['CHROMIUM', 'CHROMIUM_ANDROID'] ?
> Tools/TestResultServer/static-dashboards/flakiness_dashboard_unittests.js:773 > + var trie = new TestTrie(builders, resultsByBuilder); > + deepEqual(trie._trie, expectedTrie);
Mind also adding a couple test of TestTrie.forEach (one that takes a startpath and one that doesn't)?
Zan Dobersek
Comment 13
2012-10-12 14:41:08 PDT
Comment on
attachment 168403
[details]
Patch Thanks for all the reviewing! View in context:
https://bugs.webkit.org/attachment.cgi?id=168403&action=review
>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:52 >> + 'ANDROID': { fallbackPlatforms: ['CHROMIUM'], expectationsDirectory: 'chromium-android' } > > Shouldn't this be ['CHROMIUM', 'CHROMIUM_ANDROID'] ?
No, if a platform object has the expectationsDirectory member and subsequently gets the associated test expectations loaded (via requestExpectationsFiles() in dashboard_base.js), then these expectations will be applied to the platform (on the top of those expectations linked to the fallback platforms). See for instance how subplatforms of GTK or EFL do the same.
>> Tools/TestResultServer/static-dashboards/flakiness_dashboard_unittests.js:773 >> + deepEqual(trie._trie, expectedTrie); > > Mind also adding a couple test of TestTrie.forEach (one that takes a startpath and one that doesn't)?
Ugh, forgot about that. Will add.
Zan Dobersek
Comment 14
2012-10-14 13:08:54 PDT
Modified and landed in
r131254
.
http://trac.webkit.org/changeset/131254
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug