[TestResultServer] Add support for non-Chromium TestExpectations files
Created attachment 167131 [details] Patch
(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.
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
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.
Created attachment 168189 [details] Patch
(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.
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.
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?
(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.
Created attachment 168403 [details] Patch
(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.
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)?
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.
Modified and landed in r131254. http://trac.webkit.org/changeset/131254