Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2767433002:
Start using /list_tests to populate subtest menus in test-picker (Closed)
DescriptionStart using /list_tests to populate subtest menus in test-picker
Demo: https://dev-eakuefner-60c9d4af-dot-chromeperf.appspot.com/report
BUG=catapult:#3228
Review-Url: https://codereview.chromium.org/2767433002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/2c2b58de54d8788cccaddf3f3846c9fa1de00306
Patch Set 1 #
Total comments: 3
Patch Set 2 : saner approach #Patch Set 3 : working except for cleanup; need to port tests. #Patch Set 4 : done #
Total comments: 7
Patch Set 5 : Address Annie's comments #
Total comments: 2
Patch Set 6 : Address Annie's comment #Patch Set 7 : fix typo #
Messages
Total messages: 26 (11 generated)
eakuefner@chromium.org changed reviewers: + simonhatch@chromium.org, sullivan@chromium.org
Wanted to upload my partial progress here. It almost works. Question: we don't currently have a way to surface the deprecated annotation for individual subtests with this approach because deprecated came from the subtest dict. To reconcile this, I'm thinking about adding a "return_deprecated" query parameter to the test_path_dict mode for list_tests and doing a pair of XHRs when we query for subtests. As far as closing out this CL, I'd like to talk in 1:1s about the challenges of working with this code and our philosophy for dealing with these kinds of issues as we continue to improve the codebase. I have a feeling that more promises and a more intentionally designed state machine would make the picker much easier to understand and modify, and I want work on better understanding how we balance moving forward and cleaning up messes.
On 2017/03/20 23:47:11, eakuefner wrote: > Wanted to upload my partial progress here. It almost works. > > Question: we don't currently have a way to surface the deprecated annotation for > individual subtests with this approach because deprecated came from the subtest > dict. To reconcile this, I'm thinking about adding a "return_deprecated" query > parameter to the test_path_dict mode for list_tests and doing a pair of XHRs > when we query for subtests. > > As far as closing out this CL, I'd like to talk in 1:1s about the challenges of > working with this code and our philosophy for dealing with these kinds of issues > as we continue to improve the codebase. I have a feeling that more promises and > a more intentionally designed state machine would make the picker much easier to > understand and modify, and I want work on better understanding how we balance > moving forward and cleaning up messes. I haven't looked at the CL yet, but I think we should drop the "deprecated" feature as it currently exists, for two reasons: 1) It doesn't work properly, only a few tests on the S5 bot are deprecated for example: https://github.com/catapult-project/catapult/issues/3365#issuecomment-285772787 2) When we do fix it, we'll likely want to have a very different data model for it. So let's not block on deprecated.
Overall this looks good but: * Do you have a plan for testing? * The loop was pretty confusing to me, see comment. https://codereview.chromium.org/2767433002/diff/1/dashboard/dashboard/element... File dashboard/dashboard/elements/test-picker.html (right): https://codereview.chromium.org/2767433002/diff/1/dashboard/dashboard/element... dashboard/dashboard/elements/test-picker.html:323: this.getCurrentPreselectedPath()); This is pretty confusing. It seems like either: * We're making the same XHR (selectionModels.length - startIndex) times -or- * Something is updated in this loop that makes getCurrentPreselectedPath return a different value each time. I can't tell which it is. What's going on here?
Not super familiar with js async/await, they look kinda neat though. Remind me of the tasklet stuff we just started playing with. What's left after this? https://codereview.chromium.org/2767433002/diff/1/dashboard/dashboard/element... File dashboard/dashboard/elements/test-picker.html (right): https://codereview.chromium.org/2767433002/diff/1/dashboard/dashboard/element... dashboard/dashboard/elements/test-picker.html:323: this.getCurrentPreselectedPath()); On 2017/03/21 13:24:23, sullivan wrote: > This is pretty confusing. It seems like either: > > * We're making the same XHR (selectionModels.length - startIndex) times > -or- > * Something is updated in this loop that makes getCurrentPreselectedPath return > a different value each time. > > I can't tell which it is. What's going on here? Yeah this could use some clarification, had to pass over this a few times.
https://codereview.chromium.org/2767433002/diff/1/dashboard/dashboard/element... File dashboard/dashboard/elements/test-picker.html (right): https://codereview.chromium.org/2767433002/diff/1/dashboard/dashboard/element... dashboard/dashboard/elements/test-picker.html:303: updateSubtestMenus: async function(startIndex) { Just for record-keeping since we spoke offline, simple_xhr can be aborted, need to think about how to handle cases like first action is slow, user selects a 2nd, which comes back faster, then first one comes back and mucks up the UI.
Just to update, I think I've got the basic structure down finally, and I just need to go through and do a long list of small stuff (need to mangle the response from /list_tests a little bit, need to implement some small helpers, etc.). Because I'm using the pattern that I describe in https://github.com/catapult-project/catapult/issues/3441, I expect to be able to port the tests pretty straightforwardly as well, not to mention being able to get rid of the xhr mocks from the tests. As to Simon's point about what happens if the user interacts while the update is in progress, I also want to implement cancellation but I think that might be able to be a property of the memoization table. In particular, I'm thinking that rather than adding simple_xhr.asPromise directly to the map, we instead should be able to Promise.race() that promise with another one that listens for a call to a cancel method. I might not wait to implement that bit to seek more feedback. But anyway, stay tuned.
Description was changed from ========== [WIP] Start using /list_tests to populate subtest menus in test-picker BUG=catapult:# ========== to ========== [WIP] Start using /list_tests to populate subtest menus in test-picker ==========
Description was changed from ========== [WIP] Start using /list_tests to populate subtest menus in test-picker ========== to ========== Start using /list_tests to populate subtest menus in test-picker Demo: https://dev-eakuefner-60c9d4af-dot-chromeperf.appspot.com/report ==========
Please take a look. Unfortunately the tests are busted as explained below. https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... File dashboard/dashboard/elements/test-picker-test.html (right): https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/test-picker-test.html:138: botMenu.selectedItem = botMenu.items[1]; The problem with this is, mutating selectedItem like this causes Polymer to call onDropdownSelect which then calls updateSubtestMenus, which is asynchronous and causes the benchmark menu to be populated. Hence, below when we say let benchmarkMenu = testPicker.getSelectionMenu(2), it doesn't return the would-be second menu. Previously, this didn't matter, because the XHR mocks worked synchronously. I'm not sure what the best way to do this, because we don't have access to the promise returned by updateSubtestMenus so we can't await it. We could call updateSubtestMenus explicitly here, in which case we would duplicate effort but be able to explicitly await that call, but I'm also wondering if this is a discussion we should punt, at the cost of this bit of test coverage.
lgtm looks good, but have some comments. https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... File dashboard/dashboard/elements/test-picker-test.html (right): https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/test-picker-test.html:138: botMenu.selectedItem = botMenu.items[1]; On 2017/04/13 23:26:03, eakuefner wrote: > The problem with this is, mutating selectedItem like this causes Polymer to call > onDropdownSelect which then calls updateSubtestMenus, which is asynchronous and > causes the benchmark menu to be populated. Hence, below when we say let > benchmarkMenu = testPicker.getSelectionMenu(2), it doesn't return the would-be > second menu. Previously, this didn't matter, because the XHR mocks worked > synchronously. > > I'm not sure what the best way to do this, because we don't have access to the > promise returned by updateSubtestMenus so we can't await it. We could call > updateSubtestMenus explicitly here, in which case we would duplicate effort but > be able to explicitly await that call, but I'm also wondering if this is a > discussion we should punt, at the cost of this bit of test coverage. 2 things: 1) I think it's okay to expicitly call updateSubtestMenus in order to keep this test passing, because this is a feature that has broken in the past. 2) Since you re-implemented to have a memoization pattern, shouldn't there be a simple test that uses it as intended? https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... File dashboard/dashboard/elements/test-picker.html (right): https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/test-picker.html:84: // TODO(eakuefner): https://github.com/catapult-project/catapult/issues/3441 Should add a comment as well: generalize this request memoization pattern https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/test-picker.html:94: return await this.subtests_.get(path); I'm confused. subtests_ is a Map. this.subtests_.get() should always be synchronous, right? Why is there an await keyword here?
Description was changed from ========== Start using /list_tests to populate subtest menus in test-picker Demo: https://dev-eakuefner-60c9d4af-dot-chromeperf.appspot.com/report ========== to ========== Start using /list_tests to populate subtest menus in test-picker Demo: https://dev-eakuefner-60c9d4af-dot-chromeperf.appspot.com/report BUG=catapult:#3228 ==========
Please take another look. https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... File dashboard/dashboard/elements/test-picker-test.html (right): https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/test-picker-test.html:138: botMenu.selectedItem = botMenu.items[1]; On 2017/04/14 at 00:37:30, sullivan wrote: > On 2017/04/13 23:26:03, eakuefner wrote: > > The problem with this is, mutating selectedItem like this causes Polymer to call > > onDropdownSelect which then calls updateSubtestMenus, which is asynchronous and > > causes the benchmark menu to be populated. Hence, below when we say let > > benchmarkMenu = testPicker.getSelectionMenu(2), it doesn't return the would-be > > second menu. Previously, this didn't matter, because the XHR mocks worked > > synchronously. > > > > I'm not sure what the best way to do this, because we don't have access to the > > promise returned by updateSubtestMenus so we can't await it. We could call > > updateSubtestMenus explicitly here, in which case we would duplicate effort but > > be able to explicitly await that call, but I'm also wondering if this is a > > discussion we should punt, at the cost of this bit of test coverage. > > 2 things: > > 1) I think it's okay to expicitly call updateSubtestMenus in order to keep this test passing, because this is a feature that has broken in the past. Done. > 2) Since you re-implemented to have a memoization pattern, shouldn't there be a simple test that uses it as intended? Added a test that uses an XHR mock to test fetching behavior, a test that tests prepopulate, and finally a smoke test that ensures that Subtests stores promises. https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... File dashboard/dashboard/elements/test-picker.html (right): https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/test-picker.html:84: // TODO(eakuefner): https://github.com/catapult-project/catapult/issues/3441 On 2017/04/14 at 00:37:30, sullivan wrote: > Should add a comment as well: generalize this request memoization pattern Done. https://codereview.chromium.org/2767433002/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/test-picker.html:94: return await this.subtests_.get(path); On 2017/04/14 at 00:37:30, sullivan wrote: > I'm confused. subtests_ is a Map. this.subtests_.get() should always be synchronous, right? Why is there an await keyword here? Oops, great catch! I actually meant for the promise to be stored _in_ the map, which is the whole point of this pattern. I made a mistake below by awaiting the XHR before doing the rest of the work, because this actually returns a promise that stores the result in the map, which is not correct. I've factored out this bit into a private async helper that I now call here so that we can both store the promise in the map and await it before returning. I've also added a test to ensure that this class stores promises.
lgtm lgtm, but a question about naming below https://codereview.chromium.org/2767433002/diff/80001/dashboard/dashboard/ele... File dashboard/dashboard/elements/test-picker.html (right): https://codereview.chromium.org/2767433002/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/test-picker.html:89: this.subtests_ = new Map(); The naming here is still pretty confusing to me, as someone looking at a codebase which is not using promises yet much. If you named these like: this.subtestPromises_ const subtestPromises It would be a lot easier to understand lines like return await this.subtestPromises_.get(path);
https://codereview.chromium.org/2767433002/diff/80001/dashboard/dashboard/ele... File dashboard/dashboard/elements/test-picker.html (right): https://codereview.chromium.org/2767433002/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/test-picker.html:89: this.subtests_ = new Map(); On 2017/04/14 at 17:42:17, sullivan wrote: > The naming here is still pretty confusing to me, as someone looking at a codebase which is not using promises yet much. If you named these like: > > this.subtestPromises_ > const subtestPromises > > It would be a lot easier to understand lines like > > return await this.subtestPromises_.get(path); Done. I think that, once generalized, this will be much clearer anyway (probably the map will be called 'promises_' :).
The CQ bit was checked by eakuefner@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sullivan@chromium.org Link to the patchset: https://codereview.chromium.org/2767433002/#ps100001 (title: "Address Annie's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...) Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by eakuefner@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sullivan@chromium.org Link to the patchset: https://codereview.chromium.org/2767433002/#ps120001 (title: "fix typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492193041359170,
"parent_rev": "ab1916d41a751274a82dc26110a1660e732830e8", "commit_rev":
"2c2b58de54d8788cccaddf3f3846c9fa1de00306"}
Message was sent while issue was closed.
Description was changed from ========== Start using /list_tests to populate subtest menus in test-picker Demo: https://dev-eakuefner-60c9d4af-dot-chromeperf.appspot.com/report BUG=catapult:#3228 ========== to ========== Start using /list_tests to populate subtest menus in test-picker Demo: https://dev-eakuefner-60c9d4af-dot-chromeperf.appspot.com/report BUG=catapult:#3228 Review-Url: https://codereview.chromium.org/2767433002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
