New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 647115 link

Starred by 1 user
Status: Fixed
Owner:
█ tsergeant@chromium.org
Not on Chrome
Closed: Jun 2017
Cc:
dpa...@chromium.org
█ dschuyler@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[MD WebUI] Improve reliability and friendliness of test suite

Project Member Reported by tsergeant@chromium.org, Sep 15 2016 
Planned changes:

* Convert tests to use replaceApp() to prevent global state persisting between tests
* Where appropriate, make tests more unit-testy by creating a single instance of the element being tested
* Remove the registerTests() 'namespacing'
* Ensure tests are in the appropriate suite
* Remove excessive uses of the flush() promise method, replacing them with Polymer.dom.flush() where possible
* Try using the BrowserProxy testing pattern rather than relying on registerMessageCallback() (see: https://docs.google.com/document/d/1c20VYdwpUPyBRQeAS0CMr6ahwWnb0s26gByomOwqDjk/edit)
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 14 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b6d611908cf8e91a45ef5edd3dfdb9d714a0baf9

commit b6d611908cf8e91a45ef5edd3dfdb9d714a0baf9
Author: tsergeant <tsergeant@chromium.org>
Date: Mon Nov 14 22:39:59 2016

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

Review-Url: https://codereview.chromium.org/2404763002
Cr-Commit-Position: refs/heads/master@{#431923}

[modify] https://crrev.com/b6d611908cf8e91a45ef5edd3dfdb9d714a0baf9/chrome/browser/resources/md_history/app.crisper.js
[modify] https://crrev.com/b6d611908cf8e91a45ef5edd3dfdb9d714a0baf9/chrome/browser/resources/md_history/app.js
[modify] https://crrev.com/b6d611908cf8e91a45ef5edd3dfdb9d714a0baf9/chrome/test/data/webui/md_history/history_list_test.js
[modify] https://crrev.com/b6d611908cf8e91a45ef5edd3dfdb9d714a0baf9/chrome/test/data/webui/md_history/md_history_browsertest.js
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 25 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b3451f9f323fed4630432f989fa6c678089d96f8

commit b3451f9f323fed4630432f989fa6c678089d96f8
Author: tsergeant <tsergeant@chromium.org>
Date: Fri Nov 25 04:35:48 2016

MD History: Remove unnecessary namespacing from tests (part 1)

These tests followed the (outdated) practice of defining tests
inside a dedicated `registerTests` function. This no longer has any
benefit for isolating tests, and just reduces the amount of horizontal
space available for test code.

This CL only removes these registerTests functions and deindents
the corresponding code -- there are no functional changes to any
tests.

BUG= 647115 

Review-Url: https://codereview.chromium.org/2504813002
Cr-Commit-Position: refs/heads/master@{#434433}

[modify] https://crrev.com/b3451f9f323fed4630432f989fa6c678089d96f8/chrome/test/data/webui/md_history/history_item_test.js
[modify] https://crrev.com/b3451f9f323fed4630432f989fa6c678089d96f8/chrome/test/data/webui/md_history/history_list_test.js
[modify] https://crrev.com/b3451f9f323fed4630432f989fa6c678089d96f8/chrome/test/data/webui/md_history/history_metrics_test.js
[modify] https://crrev.com/b3451f9f323fed4630432f989fa6c678089d96f8/chrome/test/data/webui/md_history/history_overflow_menu_test.js
[modify] https://crrev.com/b3451f9f323fed4630432f989fa6c678089d96f8/chrome/test/data/webui/md_history/history_supervised_user_test.js
[modify] https://crrev.com/b3451f9f323fed4630432f989fa6c678089d96f8/chrome/test/data/webui/md_history/history_synced_tabs_test.js
[modify] https://crrev.com/b3451f9f323fed4630432f989fa6c678089d96f8/chrome/test/data/webui/md_history/md_history_browsertest.js

Comment 3 by dbeam@chromium.org, Jan 9 2017

Cc: dschuyler@chromium.org dpa...@chromium.org
Summary: [MD WebUI] Improve reliability and friendliness of test suite (was: [MD History] Improve reliability and friendliness of test suite)
it has recently come up as to whether the whole "registerTests()" namespacing and registration is needed in md-settings' tests as well.

to be perfectly clear: it's never useful to add extra namespacing.  in almost all cases, extra namespaces / registration only adds cruft and provides no benefit as pretty much every _test.js file is run in isolation.  though it's possible this may change in the future, RIGHT NOW it's better just to drop this stuff.

soooo

  // my_test.js
  cr.define('overly_long_namespace', function() {
    function registerTests() {
      ... real code starts here ...
    }
    return {
      registerTests: registerTests,
    },
  });

  // cr_settings_browsertest.js
  TEST_F('BrowserTestGroupName', 'TestCaseName', function() {
    overly_long_namespace.registerTests();
    mocha.run();
  })

should become (and has, generally, in newer tests):

  //  my_test.js
  ... code starts here ...

  // cr_settings_browsertest.js
  TEST_F('BrowserTestGroupName', 'TestCaseName', function() {
    mocha.run();
  });

until concretely proven otherwise useful.

Comment 4 by dbeam@chromium.org, Jan 10 2017 

it's never useful to add unnecessary** namespacing
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 12 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1a17a732d52a25fc250243aa4b968de6b3d50739

commit 1a17a732d52a25fc250243aa4b968de6b3d50739
Author: tsergeant <tsergeant@chromium.org>
Date: Thu Jan 12 22:07:30 2017

MD History: Remove unnecessary namespacing from tests (part 2)

These files define test cases inside a dedicated `registerTests`
function. This no longer has any benefit for isolating tests, and just
reduces the amount of horizontal space available for test code.

This CL only removes these registerTests functions and deindents
the corresponding code -- there are no functional changes to any
tests.

BUG= 647115 

Review-Url: https://codereview.chromium.org/2627263002
Cr-Commit-Position: refs/heads/master@{#443380}

[modify] https://crrev.com/1a17a732d52a25fc250243aa4b968de6b3d50739/chrome/test/data/webui/md_history/browser_service_test.js
[modify] https://crrev.com/1a17a732d52a25fc250243aa4b968de6b3d50739/chrome/test/data/webui/md_history/history_drawer_test.js
[modify] https://crrev.com/1a17a732d52a25fc250243aa4b968de6b3d50739/chrome/test/data/webui/md_history/history_grouped_list_test.js
[modify] https://crrev.com/1a17a732d52a25fc250243aa4b968de6b3d50739/chrome/test/data/webui/md_history/history_toolbar_test.js
[modify] https://crrev.com/1a17a732d52a25fc250243aa4b968de6b3d50739/chrome/test/data/webui/md_history/md_history_browsertest.js

Comment 6 by tsergeant@chromium.org, Jun 29 2017

Status: Fixed (was: Assigned)
This is as fixed as it's ever going to be. MD History tests have been split apart into smaller chunks and generally use replaceApp()/replaceBody() wherever possible.
►

Sign in to add a comment