Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
28903008:
rebaseline_server: add tabs, and ability to submit new baselines to the server (Closed)
|
Created:
7 years, 2 months ago by epoger Modified:
7 years, 2 months ago CC:
skia-review_googlegroups.com, borenet, scroggo, jcgregorio, djsollen Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
Descriptionrebaseline_server: add tabs, and ability to submit new baselines to the server
Tabs allow the user to divide the tests into groups:
hide these for now, approve these, etc.
(SkipBuildbotRuns)
R=borenet@google.com
Committed: https://code.google.com/p/skia/source/detail?r=11915
Patch Set 1 #Patch Set 2 : tabs_to_spaces #
Total comments: 6
Patch Set 3 : make_server_handle_edits #
Total comments: 7
Patch Set 4 : misc #
Total comments: 8
Patch Set 5 : more #
Messages
Total messages: 10 (0 generated)
Ready for review at patchset 2. Reviewers: I invite each of you to comment on UI, code, or both as you see fit. You can check it out at: $ ./server.py --export --port 8001 --editable http://wpgntat-ubiq141.hot.corp.google.com:8001/static/view.html?resultsToLoa... $ ./server.py --export --port 8002 --reload 60 --expectations-dir .expectations http://wpgntat-ubiq141.hot.corp.google.com:8002/static/view.html?resultsToLoa... https://codereview.chromium.org/28903008/diff/30001/gm/rebaseline_server/stat... File gm/rebaseline_server/static/loader.js (right): https://codereview.chromium.org/28903008/diff/30001/gm/rebaseline_server/stat... gm/rebaseline_server/static/loader.js:59: ['Pending Approval']); So, if run without --editable, you still have two tabs: Unfiled and Hidden. This allows the user to hide test results that are merely a distraction. On the other hand, now that the checkboxes show up even if --editable was not set, it makes it harder for the user to know whether he will be able to approve results within the UI. (The only difference right now is the missing Pending Approval tab.) Any thoughts on this? https://codereview.chromium.org/28903008/diff/30001/gm/rebaseline_server/stat... gm/rebaseline_server/static/loader.js:67: $scope.testData[i].tab = $scope.defaultTab; Each test result belongs to a single tab. https://codereview.chromium.org/28903008/diff/30001/gm/rebaseline_server/stat... gm/rebaseline_server/static/loader.js:91: return (-1 != $scope.selectedItems.indexOf(index)); Maintaining $scope.isItemSelected as an array made the other changes in this CL easier. https://codereview.chromium.org/28903008/diff/30001/gm/rebaseline_server/stat... gm/rebaseline_server/static/loader.js:176: $filter("filter")( In the tabs other than "Unfiled", we don't allow the user to hide tests by name, config, etc. (We want the user to know exactly which tests she has filed as Pending Approval) https://codereview.chromium.org/28903008/diff/30001/gm/rebaseline_server/stat... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/28903008/diff/30001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:25: <div style="background-color:#bbffbb"><!-- TODOs --> The TODO section just moved up from below. https://codereview.chromium.org/28903008/diff/30001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:68: <div class="tab-{{tab == viewingTab}}" World's ugliest tabs. But they work.
Eric: please review the code (and UI if you want to) Brian: please review the UI (and code if you want to) Please review at patchset 3, which now supports submission of new baselines on the server side. All my notes from patchset 2 still apply, including links to where you can play with the UI (and update expectations within my local checkout!)
On 2013/10/22 18:08:17, epoger wrote: > Brian: please review the UI (and code if you want to) Why are the filters on the first tab instead of global? If you're not planning to add a bunch of tabs it might make sense to have dedicated buttons to move to each other tab (e.g. Move To Hidden and Move To PendingApproval on the Unfiled tab). I don't see an immediate need now, but I wonder if an All tab would make sense (shows all the results regardless of what tab they're in and maybe lists what tab each is in).
The code looks mostly good, but I have some security concerns. Having the server edit files is scary! https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/resu... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:96: image_name = IMAGE_FILENAME_FORMATTER % (mod['test'], mod['config']) Should we verify that there are no funny characters in the input from the website which might cause us to modify files other than the ones we're expecting to modify? https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:110: Results._write_dicts_to_root(expected_builder_dicts, self._expected_root) I'm scared of what happens when multiple users try to update expectations simultaneously. Do we need some kind of synchronization? https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/serv... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:286: dispatcher = dispatchers[normpath] Should you use dispatchers.get(..) instead so that you can provide a default? That, or put this in the try/except?
Thanks for the suggestions! Please see patchset 4. https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/resu... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:96: image_name = IMAGE_FILENAME_FORMATTER % (mod['test'], mod['config']) On 2013/10/22 19:33:16, borenet wrote: > Should we verify that there are no funny characters in the input from the > website which might cause us to modify files other than the ones we're expecting > to modify? Added a "Security note" to _write_dicts_to_root(). I think we're ok. Thanks for asking. https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:110: Results._write_dicts_to_root(expected_builder_dicts, self._expected_root) On 2013/10/22 19:33:16, borenet wrote: > I'm scared of what happens when multiple users try to update expectations > simultaneously. Do we need some kind of synchronization? If you mean multiple users committing edits to the same rebaseline_server: the oldResultsHash check (see line 336 of server.py) should prevent multiple users' updates from interfering with each other. (The first one to submit his edits will win; the others will have to recreate them.) (And besides, we won't have a common server that multiple developers submit rebaselines to, in the near term. Instead, each developer will launch his own rebaseline_server for now. Hopefully we'll figure out a way that developers don't have to launch rebaseline_server or manually commit their baselines to svn/git, but we're not there yet.) If you mean multiple users, each committing edits to his own rebaseline_server, and the clash happens when they commit to the repo: that's outside the scope of this tool. Hopefully, svn/git will handle it reasonably well. https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/serv... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/serv... gm/rebaseline_server/server.py:286: dispatcher = dispatchers[normpath] On 2013/10/22 19:33:16, borenet wrote: > Should you use dispatchers.get(..) instead so that you can provide a default? > That, or put this in the try/except? Good catch. I put it in the try block. https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/res... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/res... gm/rebaseline_server/results.py:89: TODO(epoger): For now, this does not allow the caller to set any fields On 2013/10/22 19:33:16, borenet wrote: > The code looks mostly good, but I have some security concerns. Having the server > edit files is scary! I think your fear is well-placed. I think we are in relatively good shape (details below), and besides, I think developers will typically run this WITHOUT the --export flag when rebaselining. But I welcome your continued pushback if you think there are holes we need to address. https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/sta... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/sta... gm/rebaseline_server/static/view.html:68: {{tab}} ({{numResultsPerTab[tab]}}) On 2013/10/22 18:19:14, bsalomon wrote: > On 2013/10/22 18:08:17, epoger wrote: > > > Brian: please review the UI (and code if you want to) > > Why are the filters on the first tab instead of global? I figured the typical rebaselining workflow would go like this: 1. launch server 2. launch client (open URL in browser), viewing Unfiled tab 3. iterate over the list of failed and/or failure-ignored tests, shunting individual test results to Hidden or Pending Approval depending on whether they pertain to my code change 4. if the list of failed tests is long, I will probably use the filters to make step 3 easier 5. when I think I've got them all, go to the Pending Approval tab 6. take one last look at the results in the Pending Approval tab, and assuming I'm comfortable with all of them (maybe calling over another developer to look), submit them If this is a typical workflow, than my guess is that we don't want a filtered view on the Pending Approval tab (because I want to be absolutely clear on the set of rebaselines I am committing, and know that there aren't any hidden from view) Also, one of the reasons that filtering is NECESSARY in the Unfiled tab is that there are often so many entries there. I think that won't typically be the case for the Pending Approval tab. So those are my thoughts on the matter... but I think you know better than I do what a typical workflow will be. So I would be happy to change the behavior as you see fit. Please tell me how you'd like it to be, or we can look at it together tomorrow if you like (I'll be in the office). > > If you're not planning to add a bunch of tabs it might make sense to have > dedicated buttons to move to each other tab (e.g. Move To Hidden and Move To > PendingApproval on the Unfiled tab). Great idea! Done. > I don't see an immediate need now, but I wonder if an All tab would make sense > (shows all the results regardless of what tab they're in and maybe lists what > tab each is in). How about this compromise, at least for now: the tabs now show the number of results available at each tab. (So you can see at a glance where the results are)
On 2013/10/22 21:17:45, epoger wrote: > Thanks for the suggestions! Please see patchset 4. > > https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/resu... > File gm/rebaseline_server/results.py (right): > > https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/resu... > gm/rebaseline_server/results.py:96: image_name = IMAGE_FILENAME_FORMATTER % > (mod['test'], mod['config']) > On 2013/10/22 19:33:16, borenet wrote: > > Should we verify that there are no funny characters in the input from the > > website which might cause us to modify files other than the ones we're > expecting > > to modify? > > Added a "Security note" to _write_dicts_to_root(). I think we're ok. Thanks > for asking. > > https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/resu... > gm/rebaseline_server/results.py:110: > Results._write_dicts_to_root(expected_builder_dicts, self._expected_root) > On 2013/10/22 19:33:16, borenet wrote: > > I'm scared of what happens when multiple users try to update expectations > > simultaneously. Do we need some kind of synchronization? > > If you mean multiple users committing edits to the same rebaseline_server: the > oldResultsHash check (see line 336 of server.py) should prevent multiple users' > updates from interfering with each other. (The first one to submit his edits > will win; the others will have to recreate them.) > > (And besides, we won't have a common server that multiple developers submit > rebaselines to, in the near term. Instead, each developer will launch his own > rebaseline_server for now. Hopefully we'll figure out a way that developers > don't have to launch rebaseline_server or manually commit their baselines to > svn/git, but we're not there yet.) > > If you mean multiple users, each committing edits to his own rebaseline_server, > and the clash happens when they commit to the repo: that's outside the scope of > this tool. Hopefully, svn/git will handle it reasonably well. > > https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/serv... > File gm/rebaseline_server/server.py (right): > > https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/serv... > gm/rebaseline_server/server.py:286: dispatcher = dispatchers[normpath] > On 2013/10/22 19:33:16, borenet wrote: > > Should you use dispatchers.get(..) instead so that you can provide a default? > > That, or put this in the try/except? > > Good catch. I put it in the try block. > > https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/res... > File gm/rebaseline_server/results.py (right): > > https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/res... > gm/rebaseline_server/results.py:89: TODO(epoger): For now, this does not allow > the caller to set any fields > On 2013/10/22 19:33:16, borenet wrote: > > The code looks mostly good, but I have some security concerns. Having the > server > > edit files is scary! > > I think your fear is well-placed. I think we are in relatively good shape > (details below), and besides, I think developers will typically run this WITHOUT > the --export flag when rebaselining. But I welcome your continued pushback if > you think there are holes we need to address. > > https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/sta... > File gm/rebaseline_server/static/view.html (right): > > https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/sta... > gm/rebaseline_server/static/view.html:68: {{tab}} > ({{numResultsPerTab[tab]}}) > On 2013/10/22 18:19:14, bsalomon wrote: > > On 2013/10/22 18:08:17, epoger wrote: > > > > > Brian: please review the UI (and code if you want to) > > > > Why are the filters on the first tab instead of global? > > I figured the typical rebaselining workflow would go like this: > 1. launch server > 2. launch client (open URL in browser), viewing Unfiled tab > 3. iterate over the list of failed and/or failure-ignored tests, shunting > individual test results to Hidden or Pending Approval depending on whether they > pertain to my code change > 4. if the list of failed tests is long, I will probably use the filters to make > step 3 easier > 5. when I think I've got them all, go to the Pending Approval tab > 6. take one last look at the results in the Pending Approval tab, and assuming > I'm comfortable with all of them (maybe calling over another developer to look), > submit them > > If this is a typical workflow, than my guess is that we don't want a filtered > view on the Pending Approval tab (because I want to be absolutely clear on the > set of rebaselines I am committing, and know that there aren't any hidden from > view) > > Also, one of the reasons that filtering is NECESSARY in the Unfiled tab is that > there are often so many entries there. I think that won't typically be the case > for the Pending Approval tab. > > So those are my thoughts on the matter... but I think you know better than I do > what a typical workflow will be. So I would be happy to change the behavior as > you see fit. Please tell me how you'd like it to be, or we can look at it > together tomorrow if you like (I'll be in the office). > I was thinking that the filters were applied to all the tabs, but if only the unfiled tab is filtered then that makes sense. And it seems right to me to show all the entries in the other two tabs all the time. > > > > If you're not planning to add a bunch of tabs it might make sense to have > > dedicated buttons to move to each other tab (e.g. Move To Hidden and Move To > > PendingApproval on the Unfiled tab). > > Great idea! Done. > > > I don't see an immediate need now, but I wonder if an All tab would make sense > > (shows all the results regardless of what tab they're in and maybe lists what > > tab each is in). > > How about this compromise, at least for now: the tabs now show the number of > results available at each tab. (So you can see at a glance where the results > are) SGTM
Code LGTM with a couple more comments. https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/resu... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/28903008/diff/70001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:110: Results._write_dicts_to_root(expected_builder_dicts, self._expected_root) On 2013/10/22 21:17:45, epoger wrote: > On 2013/10/22 19:33:16, borenet wrote: > > I'm scared of what happens when multiple users try to update expectations > > simultaneously. Do we need some kind of synchronization? > > If you mean multiple users committing edits to the same rebaseline_server: the > oldResultsHash check (see line 336 of server.py) should prevent multiple users' > updates from interfering with each other. (The first one to submit his edits > will win; the others will have to recreate them.) > > (And besides, we won't have a common server that multiple developers submit > rebaselines to, in the near term. Instead, each developer will launch his own > rebaseline_server for now. Hopefully we'll figure out a way that developers > don't have to launch rebaseline_server or manually commit their baselines to > svn/git, but we're not there yet.) > > If you mean multiple users, each committing edits to his own rebaseline_server, > and the clash happens when they commit to the repo: that's outside the scope of > this tool. Hopefully, svn/git will handle it reasonably well. > Gotcha. I was envisioning this as a single server that everyone shares. Running it on your own machine avoids a lot of my concerns. https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/res... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/res... gm/rebaseline_server/results.py:196: could contain poisonous data. Thanks! We can feel somewhat secure since we're using the json module which should fail on malformed data. https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/res... gm/rebaseline_server/results.py:216: continue We don't have any expectations with "-Trybot". Do we expect to? This check seems unnecessary, or even undesirable if we ever ended up having separate files for the trybots - we couldn't rebaseline them. https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/ser... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/ser... gm/rebaseline_server/server.py:119: """ Any reason not to move this implementation into update_results? As it stands, this function can be called without the lock. Trust nobody!
Eric, please see changes in patchset 5, and let me know if I should go ahead and commit, or we can discuss any remaining issues live. Brian, if you have any final comments, or want to discuss live, let me know. https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/res... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/res... gm/rebaseline_server/results.py:216: continue On 2013/10/23 14:22:03, borenet wrote: > We don't have any expectations with "-Trybot". Do we expect to? This check seems > unnecessary, or even undesirable if we ever ended up having separate files for > the trybots - we couldn't rebaseline them. Added some comments here and in _read_dicts_from_root(). https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/ser... File gm/rebaseline_server/server.py (right): https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/ser... gm/rebaseline_server/server.py:119: """ On 2013/10/23 14:22:03, borenet wrote: > Any reason not to move this implementation into update_results? As it stands, > this function can be called without the lock. Trust nobody! No reason at all! I was just trying to reduce the apparent complexity of the CL (by avoiding the indentation change below). Fixed.
LGTM at patch set 5, although I'm getting a 404 on view.css. I'll just trust that nothing nasty happened in the last patch set! https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/res... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/28903008/diff/230001/gm/rebaseline_server/res... gm/rebaseline_server/results.py:216: continue On 2013/10/23 14:42:14, epoger wrote: > On 2013/10/23 14:22:03, borenet wrote: > > We don't have any expectations with "-Trybot". Do we expect to? This check > seems > > unnecessary, or even undesirable if we ever ended up having separate files for > > the trybots - we couldn't rebaseline them. > > Added some comments here and in _read_dicts_from_root(). Thanks!
Message was sent while issue was closed.
Committed patchset #5 manually as r11915 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
