| Summary: | Make rebaseline server usable with bot results | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mihai Parparita <mihaip> | ||||||||||||
| Component: | Tools / Tests | Assignee: | Dirk Pranke <dpranke> | ||||||||||||
| Status: | RESOLVED WONTFIX | ||||||||||||||
| Severity: | Normal | CC: | abarth, dpranke, eric, ojan, tony, vangelis | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Mihai Parparita
2011-01-06 17:56:20 PST
Created attachment 78195 [details]
Patch
Comment on attachment 78195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78195&action=review Seems this needs at least one more round. > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:112 > + if not path.endswith('/'): Do we need to normalize the path? symlinks? ..? foo/bar// etc? > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:171 > + def get_result_path(suffix): We don't normally use get_ in names, right? > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:173 > + return test_config.filesystem.join( > + test_config.results_directory, test_name + suffix) Most our code doesn't wory about 80c, but it's not a deal breaker. > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:189 > + results_expected_path = test_config.filesystem.join( > + test_config.results_directory, > + test_name + '-expected' + extension) > + if test_config.filesystem.isfile(results_expected_path): > + return results_expected_path > + test_path = test_config.filesystem.join( > + test_config.layout_tests_directory, test_file) > + test_baselines = test_config.test_port.expected_baselines( > + test_path, extension) > + return test_config.filesystem.join( > + test_baselines[0][0], test_baselines[0][1]) This is really hard to parse. Can't we make this easier to read? > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:198 > + file_path = get_result_path('-actual.checksum') Yeah, just drop the get_ it's awkward (and I think explicitly avoided in webkitstyle) > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:420 > +def _construct_results_json_from_directory(test_config): This feels a lot like what we do in LayoutTestResults to construct TestResults objects from a results.html file. I wonder if these concepts should be merged. > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:439 > + failure_type = _RESULT_EXTENSION_TO_FAILURE_TYPE.get( > + test_extension, 'FAIL') 80c is just hurting readability here. > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:445 > + filesystem.listdir( > + filesystem.join( > + test_config.layout_tests_directory, test_dir)), > + test_name + '.*') and here, wow. > Tools/Scripts/webkitpy/tool/commands/rebaselineserver_unittest.py:220 > + test_config = get_test_config( get_ must be google style? > Tools/Scripts/webkitpy/tool/commands/rebaselineserver_unittest.py:248 > + def test(self): A more specific name here? Comment on attachment 78195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78195&action=review >> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:420 >> +def _construct_results_json_from_directory(test_config): > > This feels a lot like what we do in LayoutTestResults to construct TestResults objects from a results.html file. I wonder if these concepts should be merged. This is fine for now for the bots that don't use NRWT. Once all the bots do use it though, we can just grab the unexpected_results.json from the bots directly. If we don't already archive them on the Chromium bots, we should. Maybe add a FIXME here to that effect? (In reply to comment #3) > (From update of attachment 78195 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78195&action=review > > >> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:420 > >> +def _construct_results_json_from_directory(test_config): > > > > This feels a lot like what we do in LayoutTestResults to construct TestResults objects from a results.html file. I wonder if these concepts should be merged. > > This is fine for now for the bots that don't use NRWT. Once all the bots do use it though, we can just grab the unexpected_results.json from the bots directly. If we don't already archive them on the Chromium bots, we should. Maybe add a FIXME here to that effect? My point was more: You've made a way of generating unexpected_results.json from the filesystem. I've made a way of generating results.json from results.html (with layouttestresults.py). Those sound somewhat similar to me and perhaps should share code/be in the same module/whatever long term... Created attachment 78267 [details]
Patch
>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:112 > Do we need to normalize the path? symlinks? ..? foo/bar// etc? This was actually from r75233, I just needed it before that patch landed (I've removed filesystem.py/filesystem_mock.py from this patch). More generally, I don't think we should add complexity to mocks unless it's actually needed for tests, otherwise we'll need tests for the mocks, etc. >> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:171 > We don't normally use get_ in names, right? Renamed this to result_path (and get_expected_path to expected_path). >> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:173 > Most our code doesn't wory about 80c, but it's not a deal breaker. Switched to less aggressive wrapping (here and elsewhere) >> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:189 > This is really hard to parse. Can't we make this easier to read? Added some whitespace and comments. >>>> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:420 >>> >>> This feels a lot like what we do in LayoutTestResults to construct TestResults objects from a results.html file. I wonder if these concepts should be merged. >> >> This is fine for now for the bots that don't use NRWT. Once all the bots do use it though, we can just grab the unexpected_results.json from the bots directly. If we don't already archive them on the Chromium bots, we should. Maybe add a FIXME here to that effect? > > My point was more: You've made a way of generating unexpected_results.json from the filesystem. I've made a way of generating results.json from results.html (with layouttestresults.py). Those sound somewhat similar to me and perhaps should share code/be in the same module/whatever long term... Ideally there would be one (Layout)TestResults class that could be serialized to/from JSON. It's too bad that results.json and unexpected_results.json are entirely different formats. I guess that's because results.json is for uploading to the server, whereas unexpected_results.json is more useful for tools such as this. Perhaps we can add an all_results.json that uses the unexpected_results.json-like format for all test results (basically a JSON alternatives to results.html) and then have both of those be archived? >> Tools/Scripts/webkitpy/tool/commands/rebaselineserver_unittest.py:220 > get_ must be google style? Do you have a preferred name? Dropping the get_ here seems confusing. >> Tools/Scripts/webkitpy/tool/commands/rebaselineserver_unittest.py:248 > A more specific name here? Renamed. (In reply to comment #6) > >> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:112 > > Do we need to normalize the path? symlinks? ..? foo/bar// etc? > > This was actually from r75233, I just needed it before that patch landed (I've removed filesystem.py/filesystem_mock.py from this patch). > > More generally, I don't think we should add complexity to mocks unless it's actually needed for tests, otherwise we'll need tests for the mocks, etc. Sorry, I didn't realize I was reading mock code at that moment. :) > Ideally there would be one (Layout)TestResults class that could be serialized to/from JSON. It's too bad that results.json and unexpected_results.json are entirely different formats. I guess that's because results.json is for uploading to the server, whereas unexpected_results.json is more useful for tools such as this. Perhaps we can add an all_results.json that uses the unexpected_results.json-like format for all test results (basically a JSON alternatives to results.html) and then have both of those be archived? I guess I wasn't aware there were two separate .json formats from NRWT. ORWT only knows how to spit out results.html (which is neither!) (In reply to comment #7) > (In reply to comment #6) > > Ideally there would be one (Layout)TestResults class that could be serialized to/from JSON. It's too bad that results.json and unexpected_results.json are entirely different formats. I guess that's because results.json is for uploading to the server, whereas unexpected_results.json is more useful for tools such as this. Perhaps we can add an all_results.json that uses the unexpected_results.json-like format for all test results (basically a JSON alternatives to results.html) and then have both of those be archived? > > I guess I wasn't aware there were two separate .json formats from NRWT. ORWT only knows how to spit out results.html (which is neither!) I think it makes sense to have two different formats. results.json does not include the actual/expected results for the test, but rather what type of failure it was, how long the test took to run, metadata about the buildbot, etc. This is the information we use for the dashboard. In practice, I don't think we use that file anymore. We use incremental_results.json, which is what we upload to appengine. So, maybe the best final result is 3 files: -incremental_results.json: remains unchanged -unexpected_results.json: remains unchanged -results.json: change it to match the format of unexpected_results.json, but for all the failures instead of just the unexpected ones. The latter two use the same format. Then finally, we should have results.html just load the JSON file and generate the results page using JS. Created attachment 78636 [details]
Patch
(In reply to comment #8) > -results.json: change it to match the format of unexpected_results.json, but for all the failures instead of just the unexpected ones. I filed bug 52267 about this, and added a FIXME in this patch to use results.json once it exists in the desired format. Any objections to getting this landed in the meantime? Comment on attachment 78636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78636&action=review > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:418 > +def _construct_results_json_from_directory(test_config): This is to work with ORWT results, right? I would just move this onto LayoutTestResults instead, since that's the class which currently deals with mapping ORWT results to the NRWT python objects. It seems like it already does some of this, but adding this functionality (even if on a separate method like this) would be useful. Then we just have one class to kill (LayoutTestResults) when our NRWT overlords subsume us. Comment on attachment 78636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78636&action=review >> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:418 >> +def _construct_results_json_from_directory(test_config): > > This is to work with ORWT results, right? I would just move this onto LayoutTestResults instead, since that's the class which currently deals with mapping ORWT results to the NRWT python objects. It seems like it already does some of this, but adding this functionality (even if on a separate method like this) would be useful. Then we just have one class to kill (LayoutTestResults) when our NRWT overlords subsume us. Mihai tells me this isn't just for ORWT, but rather NRWT as well since the bots don't currently have unexpected_results.json files. Created attachment 81166 [details]
Patch
A need for this came up again because Vangelis would like to rebaseline all the GPU results based on the GPU bots (which archive results at http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_5_-_GPU/ and similar paths). Any chance of getting this reviewed? Comment on attachment 81166 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81166&action=review Sorry this fell through the cracks. In general I'm supportive of this change. It may be fine to leave this logic on rebaseline server for now, but I definitely see other places wanting to reuse this sooner rather than later. Maybe LayoutTestResults is the right place for it? Not sure. NRWT doesn't have a "layout test results directory" object, besides perhapse Koz's new FileSet (which isn't specific to layout tests). I think in the interst of readability we should go one more round and break some of these long functions into smaller chunks. I'll try to be much faster about the next round review. My appologies again. > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:165 > + test_file = (self.query['test'][0]) Why a tuple here? > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:418 > +def _construct_results_json_from_directory(test_config): I think this should be a separate class. This is not specific to the rebaseline server. Only speicifc to the NRWT results directory format and the results.json in memeory object model. I forsee other code wanting to re-use this "generate a json object model from this on-disk directory" logic. We have LayoutTestResults which does the json object model genreation from an ORWT directory (usign results.html). One might argue thid should go there instead of a new class. Your call. I also would have broken this function into smaller pieces. It's a bit hard to read at this length. > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:502 > + print 'Parsing unexpected_results.json...' This parsing should be a helper function. No need to make execute() 3 miles long. :) Comment on attachment 81166 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81166&action=review >> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:165 >> + test_file = (self.query['test'][0]) > > Why a tuple here? Not sure, must've been a copy and paste error when I took out the param extraction from the splitext function call. Removed. >> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:418 >> +def _construct_results_json_from_directory(test_config): > > I think this should be a separate class. This is not specific to the rebaseline server. Only speicifc to the NRWT results directory format and the results.json in memeory object model. > > I forsee other code wanting to re-use this "generate a json object model from this on-disk directory" logic. > > We have LayoutTestResults which does the json object model genreation from an ORWT directory (usign results.html). One might argue thid should go there instead of a new class. Your call. > > I also would have broken this function into smaller pieces. It's a bit hard to read at this length. I'd rather not encourage use of this function by moving it to a separate class. The right solution is to include a sane results.json file in the test output, and have the archives from the bots contain that (http://webkit.org/b/52267). Reconstructing that data from the directory structure is a stop-gap measure. I've broken up the function into two to hopefully help with readability a bit. >> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:502 >> + print 'Parsing unexpected_results.json...' > > This parsing should be a helper function. No need to make execute() 3 miles long. :) Moved out parsing and the test_list blocks to separate functions. Created attachment 81310 [details]
Patch
Comment on attachment 81310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81310&action=review > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:418 > +def _construct_results_json_from_directory(test_config): Why are these all free functions? > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:435 > + os.path.dirname(result_file), results_directory) I think we generally use the filesystem. versions of these: dirname, basename, etc. But I may be remembering wrong what all filesystem has on it. > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:440 > + os.path.basename(result_file)) > + > + if file_name.endswith(_ACTUAL_SUFFIX): > + test_names.append((test_dir, file_name[:-len(_ACTUAL_SUFFIX)], result_extension)) This is basically a list comprehension. return [self._test_name_from_actual_file(actual_file) for actual_file in _actual_files_under(results_directory)] > Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:453 > + test_files = fnmatch.filter( > + filesystem.listdir(filesystem.join(test_config.layout_tests_directory, test_dir)), > + test_name + '.*') We ignore PEP8's make-your-code-fit-on-a-punch-card wrapping rule and opt to wrap for readability (which is of course subjective). This is fine, just noting. Comment on attachment 81310 [details]
Patch
I think to do this right, this needs to be an object. It's logic is wholly seperable from the rebaseline server. But it's fine to have the object live in that file if you like.
The test looks fine. You might want some finer-grain tests of the individual methods, but the full-sytem test you have looks fine.
Yes, we might delete this. But when we delete this, we'll delete the whole object and associated tests. Not some random group of free functions. :) When we always have a unexpectedresults.json file to read then we don't need ot use this code at all.
I suspect this is dead? We were actually just talking about dusting off the code for the rebaseline server and figuring out how to merge it into garden-o-matic as necessary today. Right now garden-o-matic is the preferred tool for reviewing and rebaselining a small number of live failures, and webkit-patch rebaseline-expectations is the preferred tool for rebaselining (but not reviewing) a large number of live failures. You can also use results.html to review tests that fail when run locally, but not actually rebaseline them. However, we don't have a good way of reviewing and rebaselining local failures, or reviewing and rebaselining failures from uncommitted changes but changes that come from other bots (e.g., EWS bots and/or Chromium try servers). It seems likely that some amount of the rebaseling server code would be useful here. Let me take ownership of this bug and patch and figure out what to do with it ... So, there's some interesting stuff in this patch but I largely seems to be adding things that garden-o-matic already has support for, so there doesn't seem to be a lot of point. After playing around with the rebaseline server more yesterday, I think there's definitely some things it still does better than garden-o-matic but we should make g-o-m do those things better instead of maintaining the two code bases indefinitely. I'm gonna close this as WontFix and start adding patches for that work instead. Let me know if anyone objects. |