Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2404763002:
Refactor overall test suite, use replaceApp in HistoryListTest (Closed)
|
Created:
4 years, 2 months ago by tsergeant Modified:
4 years, 1 month ago Reviewers:
calamity CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD History: Split test suite apart into separate classes
This allows us to only load the files that are needed for each specific
test, and removes the need to namespace each test. Also updates the test
names to match the style used by MD Settings and converts
MaterialHistoryListTest to use replaceApp.
BUG=647115
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b6d611908cf8e91a45ef5edd3dfdb9d714a0baf9
Cr-Commit-Position: refs/heads/master@{#431923}
Patch Set 1 : Rebase #Patch Set 2 : Undo namespacing change #
Total comments: 6
Patch Set 3 : Rename bound listeners #Patch Set 4 : Revulcanize #Patch Set 5 : Rebase?? #Patch Set 6 : Fix tests #
Messages
Total messages: 39 (30 generated)
Description was changed from ========== Refactor overall test suite, use replaceApp in HistoryListTest BUG=647115 ========== to ========== Refactor overall test suite, use replaceApp in HistoryListTest BUG=647115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Refactor overall test suite, use replaceApp in HistoryListTest BUG=647115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Split test suite apart into separate classes This allows us to only load the files that are needed for each specific test, and removes the need to namespace each test. Also updates the test names to match the style used by MD Settings and converts MaterialHistoryListTest to use replaceApp. BUG=647115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #4 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
tsergeant@chromium.org changed reviewers: + calamity@chromium.org
PTAL
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
drive-by https://codereview.chromium.org/2404763002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/app.js (right): https://codereview.chromium.org/2404763002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/app.js:121: this.onCanExecuteListener_ = this.onCanExecute_.bind(this); nit: boundOnCanExecute_ seems to be the typical polymer way (except for, you know, the _ at the end like the JS style guide likes) https://codereview.chromium.org/2404763002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_list_test.js (right): https://codereview.chromium.org/2404763002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_list_test.js:32: element = app.$['history'].$['infinite-list']; naughty https://codereview.chromium.org/2404763002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/md_history_browsertest.js (right): https://codereview.chromium.org/2404763002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_history/md_history_browsertest.js:223: md_history.history_synced_tabs_test.registerTests(); i think a good next step might be to auto-register tests (i.e. just unwrap them from a registerTests() method) and drop these lines and just have "mocha.run()"
Description was changed from ========== MD History: Split test suite apart into separate classes This allows us to only load the files that are needed for each specific test, and removes the need to namespace each test. Also updates the test names to match the style used by MD Settings and converts MaterialHistoryListTest to use replaceApp. BUG=647115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Split test suite apart into separate classes This allows us to only load the files that are needed for each specific test, and removes the need to namespace each test. Also updates the test names to match the style used by MD Settings and converts MaterialHistoryListTest to use replaceApp. BUG=647115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/2404763002/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/app.js (right): https://codereview.chromium.org/2404763002/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/app.js:121: this.onCanExecuteListener_ = this.onCanExecute_.bind(this); On 2016/11/03 18:07:51, Dan Beam wrote: > nit: boundOnCanExecute_ seems to be the typical polymer way (except for, you > know, the _ at the end like the JS style guide likes) Done. https://codereview.chromium.org/2404763002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/history_list_test.js (right): https://codereview.chromium.org/2404763002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_history/history_list_test.js:32: element = app.$['history'].$['infinite-list']; On 2016/11/03 18:07:51, Dan Beam wrote: > naughty 😓 https://codereview.chromium.org/2404763002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/md_history/md_history_browsertest.js (right): https://codereview.chromium.org/2404763002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/md_history/md_history_browsertest.js:223: md_history.history_synced_tabs_test.registerTests(); On 2016/11/03 18:07:51, Dan Beam wrote: > i think a good next step might be to auto-register tests (i.e. just unwrap them > from a registerTests() method) and drop these lines and just have "mocha.run()" Yup! I originally did this to HistoryListTest in this CL, but punted it to a follow-up.
lgtm
Patchset #5 (id:160001) has been deleted
The CQ bit was checked by tsergeant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org Link to the patchset: https://codereview.chromium.org/2404763002/#ps140001 (title: "Revulcanize")
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tsergeant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org Link to the patchset: https://codereview.chromium.org/2404763002/#ps200001 (title: "Fix tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MD History: Split test suite apart into separate classes This allows us to only load the files that are needed for each specific test, and removes the need to namespace each test. Also updates the test names to match the style used by MD Settings and converts MaterialHistoryListTest to use replaceApp. BUG=647115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Split test suite apart into separate classes This allows us to only load the files that are needed for each specific test, and removes the need to namespace each test. Also updates the test names to match the style used by MD Settings and converts MaterialHistoryListTest to use replaceApp. BUG=647115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== MD History: Split test suite apart into separate classes This allows us to only load the files that are needed for each specific test, and removes the need to namespace each test. Also updates the test names to match the style used by MD Settings and converts MaterialHistoryListTest to use replaceApp. BUG=647115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Split test suite apart into separate classes This allows us to only load the files that are needed for each specific test, and removes the need to namespace each test. Also updates the test names to match the style used by MD Settings and converts MaterialHistoryListTest to use replaceApp. BUG=647115 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b6d611908cf8e91a45ef5edd3dfdb9d714a0baf9 Cr-Commit-Position: refs/heads/master@{#431923} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b6d611908cf8e91a45ef5edd3dfdb9d714a0baf9 Cr-Commit-Position: refs/heads/master@{#431923} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
