Chrome OS autotests depend on current Settings WebUI DOM |
|||||||||||||||||||
Issue description"Could not locate section in chrome://settings to enable arc. Make sure ARC is available." https://uberchromegw.corp.google.com/i/chromeos/builders/veyron_minnie-chrome-pfq https://uberchromegw.corp.google.com/i/chromeos/builders/cyan-chrome-pfq It is failing to roll 58.0.3016 https://chromium.googlesource.com/chromium/src/+log/58.0.3015.0..58.0.3016.0?pretty=fuller&n=10000 which seems to flip MD settings https://codereview.chromium.org/2699973002 that I suspect at the first glance of the revision log. +ARC Constables
,
Feb 20 2017
+dbeam@, and +CrOSSheriffs just in case. MD settings indeed looks changing the DOM structure, so inevitably making this kind of code https://chromium.googlesource.com/chromiumos/third_party/autotest/+/master/client/common_lib/cros/arc_util.py#150 incompatible. Searching through the code, there are seems to be several other ChromeOS autotests traversing the settings page in this way, not only ARC tests.
,
Feb 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d7d491d6a26d61a06f96d8e924c54902e5c52ed4 commit d7d491d6a26d61a06f96d8e924c54902e5c52ed4 Author: kinaba <kinaba@chromium.org> Date: Tue Feb 21 01:48:50 2017 Revert of MD Settings: enable by default in client (patchset #4 id:60001 of https://codereview.chromium.org/2699973002/ ) Reason for revert: BUG= 694081 It broke ChromeOS tests and thus blocked Chrome roll to ChromeOS. In my personal opinion, what really needs fix are the tests rather than the change, but the changes to the tests looks far nontrivial. For now, allow me to revert this change to unblock the divergence between Chrome and ChromeOS ToT... Original issue's description: > MD Settings: enable by default in client > > R=dpapad@chromium.org > BUG=478982 > > Review-Url: https://codereview.chromium.org/2699973002 > Cr-Commit-Position: refs/heads/master@{#451338} > Committed: https://chromium.googlesource.com/chromium/src/+/a8109d958f144e541a204da6debd820d936d6108 TBR=dpapad@chromium.org,sky@chromium.org,dbeam@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=478982 Review-Url: https://codereview.chromium.org/2703283002 Cr-Commit-Position: refs/heads/master@{#451684} [modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/browser_about_handler_unittest.cc [modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/chrome_content_browser_client_browsertest.cc [modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc [modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/policy/policy_browsertest.cc [modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/ui/webui/options/certificate_manager_browsertest.cc [modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/ui/webui/options/language_dictionary_interactive_uitest.cc [modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/ui/webui/options/language_options_interactive_uitest.cc [modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/ui/webui/options/options_ui_browsertest.cc [modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/browser/ui/webui/options/options_ui_browsertest.h [modify] https://crrev.com/d7d491d6a26d61a06f96d8e924c54902e5c52ed4/chrome/common/chrome_features.cc
,
Feb 21 2017
(lowering to P1 since now the revert is in.)
,
Feb 21 2017
I'm feeling sorry about that, but for the health of the tree I reverted the change (as a fill-in constable of dspaid@.) Now, I'm not sure about the right next step. Passing to dbeam@. The point is, multiple Chrome OS tests are relying on injecting JS to the chrome://settings page and modifying the settings: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f75357b82a9c58434db07059ebc77edc0b3b7c73/client/common_lib/cros/arc_util.py#149 https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f75357b82a9c58434db07059ebc77edc0b3b7c73/client/site_tests/policy_AutoFillEnabled/policy_AutoFillEnabled.py#46 https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f75357b82a9c58434db07059ebc77edc0b3b7c73/client/cros/enterprise/enterprise_policy_base.py#438 ... Without making these tests robust to the settings page change, the tree will continue rejecting Chrome with MD settings.
,
Feb 21 2017
Wow. This is incredibly fragile, we really need to fix the test:
settings_tab.ExecuteJavaScript(
'assert(document.getElementById("android-apps-enabled"))'
dbeam@ - I will take ownership of this. You may want to enable MD Settings on ToT for non cros in the meanwhile.
,
Feb 21 2017
,
Feb 21 2017
Yup, we should use explicit test apis rather than parsing DOM in autotests
,
Feb 21 2017
stevenjb@: if you wouldn't just delete the test when we roll to settings, it's probably worth updating. otherwise, I'd just recommend slapping a ScopedFeatureList in that test's SetUp() (or one of the initialization methods) and calling it day meanwhile, I'm going to enable this by default everywhere but ChromeOS.
,
Feb 21 2017
dbeam@ - Unfortunately it's more complicated than that. This is a python based autotest in the chromeos code, which is why it did not show up in the waterfall. It looks like this is used by a bunch of Arc++ tests, so we need an owner and we need a more robust fix. The problems with the current mechanism were discussed here, but not followed up: https://chromium-review.googlesource.com/c/412286/
,
Feb 21 2017
+elijahtaylor@ - Any suggestions for an owner for this? We really do not want arc (or any) auto tests depending on Chrome UI code; it is very difficult to maintain (and shouldn't be something that the Chrome devs need to be concerned about).
,
Feb 21 2017
Renaming and marking available. We need an owner for this who is familiar with the arc autotests. Also upping the priority because this is preventing us from enabling MD Settings by default on CrOS.
,
Feb 21 2017
I am ARC constable this week and can take a look. What is preferable way you think of fixing this? One idea is to make it aware about different Chrome UIs (however this might be not best way).
,
Feb 21 2017
achuith@, don''t we have a set of chrome hooks for this sort of thing? All that code is doing, as near as I can tell, is setting the arc.enabled pref. We shouldn't need to execute UI code to do that.
,
Feb 22 2017
+scunningham@ Quickly code-searched. Probably the ARC test is the only tree closer, but IIUC enterprise tests are also largely depending on the settings page DOM. https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f75357b82a9c58434db07059ebc77edc0b3b7c73/client/cros/enterprise/enterprise_policy_base.py#438
,
Feb 22 2017
Scott has left the team, but I'm a good poc for Enterprise autotests. Using the settings page to enable things is a workaround for features without public APIs. The Enterprise tests in #15 are checking whether a setting was enabled/disabled after a given policy is set. A solution is we've used in the past is expanding the autotest_private to include the needed functionality: [Chrome]/src/chrome/browser/extensions/api/autotest_private/. See the touch setting functions which are used here: [Chrome OS]/src/third_party/autotest/files/client/cros/touch_playback_test_base.py
,
Feb 22 2017
For short ARC fix I suggest: https://chromium-review.googlesource.com/c/446090 This just switches chrome://settings to chrome://settings-frame (last one is not MD based). I know, this is quite hacky way but I have not found better way so far. #16 looks reasonable but this will take more time.
,
Feb 22 2017
Victor added a notifications api to autotestPrivate: https://codereview.chromium.org/1872873002 This is used by ARC tests. This is the preferred way for communication between chrome and autotests. We can have a similar treatment and introduce an api like autotestPrivate.androidAppsEnabled(). Another option that's a lot simpler is to just add a JS call to to the settings page, as I did here for the the chrome://gpu page: https://codereview.chromium.org/1430993002/ We can add a js method like AndroidAppsEnabledForTesting(), and call that from telemetry. The first solution is probably a little nicer since it doesn't involve opening the settings page, but it may be more work. I'd rather not use the chrome://settings-frame approach since it just kicks the problem down the road a bit. It's fine to land to get the tree green.
,
Feb 22 2017
Please name the API something like autotestPrivate.playStoteEnabled instead if we decide to go this way.
,
Feb 22 2017
It may be possible to just use chrome.settingsPrivate.setPref('arc.enabled', true, '', null). You may need to whitelist settingsPrivate for autotests.
Alternatively autotestPrivate.setPlayStoreEnabled(true) would also be fine.
I would very much like to see us move away from loading chrome://setings (or any WebUI page) for autotests. It is both fragile and not especially fast.
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1 commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1 Author: khmel <khmel@google.com> Date: Mon Feb 27 18:15:57 2017 arc: Fix enabling ARC from autotest This use old version chrome://settings-frame that is not MD based. BUG= chromium:694081 TEST=Manually Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8 Reviewed-on: https://chromium-review.googlesource.com/446090 Commit-Ready: Yury Khmel <khmel@google.com> Tested-by: Yury Khmel <khmel@google.com> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org> [modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py
,
Feb 27 2017
Lowering the priority, once quick fix was landed.
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1 commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1 Author: khmel <khmel@google.com> Date: Mon Feb 27 18:15:57 2017 arc: Fix enabling ARC from autotest This use old version chrome://settings-frame that is not MD based. BUG= chromium:694081 TEST=Manually Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8 Reviewed-on: https://chromium-review.googlesource.com/446090 Commit-Ready: Yury Khmel <khmel@google.com> Tested-by: Yury Khmel <khmel@google.com> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org> [modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1 commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1 Author: khmel <khmel@google.com> Date: Mon Feb 27 18:15:57 2017 arc: Fix enabling ARC from autotest This use old version chrome://settings-frame that is not MD based. BUG= chromium:694081 TEST=Manually Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8 Reviewed-on: https://chromium-review.googlesource.com/446090 Commit-Ready: Yury Khmel <khmel@google.com> Tested-by: Yury Khmel <khmel@google.com> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org> [modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py
,
Feb 27 2017
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1 commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1 Author: khmel <khmel@google.com> Date: Mon Feb 27 18:15:57 2017 arc: Fix enabling ARC from autotest This use old version chrome://settings-frame that is not MD based. BUG= chromium:694081 TEST=Manually Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8 Reviewed-on: https://chromium-review.googlesource.com/446090 Commit-Ready: Yury Khmel <khmel@google.com> Tested-by: Yury Khmel <khmel@google.com> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org> [modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1 commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1 Author: khmel <khmel@google.com> Date: Mon Feb 27 18:15:57 2017 arc: Fix enabling ARC from autotest This use old version chrome://settings-frame that is not MD based. BUG= chromium:694081 TEST=Manually Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8 Reviewed-on: https://chromium-review.googlesource.com/446090 Commit-Ready: Yury Khmel <khmel@google.com> Tested-by: Yury Khmel <khmel@google.com> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org> [modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1 commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1 Author: khmel <khmel@google.com> Date: Mon Feb 27 18:15:57 2017 arc: Fix enabling ARC from autotest This use old version chrome://settings-frame that is not MD based. BUG= chromium:694081 TEST=Manually Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8 Reviewed-on: https://chromium-review.googlesource.com/446090 Commit-Ready: Yury Khmel <khmel@google.com> Tested-by: Yury Khmel <khmel@google.com> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org> [modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py
,
Feb 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/ef2eaccbed2a30105e3b6efdeb0640923c5390f1 commit ef2eaccbed2a30105e3b6efdeb0640923c5390f1 Author: khmel <khmel@google.com> Date: Mon Feb 27 18:15:57 2017 arc: Fix enabling ARC from autotest This use old version chrome://settings-frame that is not MD based. BUG= chromium:694081 TEST=Manually Change-Id: Ic39ac038bb26ad0b8c5f6a4a3dd75133321614b8 Reviewed-on: https://chromium-review.googlesource.com/446090 Commit-Ready: Yury Khmel <khmel@google.com> Tested-by: Yury Khmel <khmel@google.com> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org> [modify] https://crrev.com/ef2eaccbed2a30105e3b6efdeb0640923c5390f1/client/common_lib/cros/arc_util.py
,
Apr 4 2017
This needs to be fixed for real in M-60 (preferably before or near the branch point) as we may be removing the old settings (settings-frame) in that dev cycle.
,
Apr 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d127ade1e89dd1b221e85170e0dbe4c32072c7c5 commit d127ade1e89dd1b221e85170e0dbe4c32072c7c5 Author: khmel <khmel@chromium.org> Date: Fri Apr 14 16:55:26 2017 arc: Provide API to control Play Store state from autotests This provides required API to read and set Play Store state from autotests since old settings page will deprecate soon and new MD settings don't provide reliable way to access Play Store settings. BUG= 694081 TEST=Manually, together with https://chromium-review.googlesource.com/c/470707/ Review-Url: https://codereview.chromium.org/2801173002 Cr-Commit-Position: refs/heads/master@{#464735} [modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/browser/chromeos/arc/arc_util.cc [modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/browser/chromeos/arc/arc_util.h [modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/browser/extensions/api/autotest_private/autotest_private_api.cc [modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/browser/extensions/api/autotest_private/autotest_private_api.h [modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/common/extensions/api/autotest_private.idl [modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/chrome/test/data/extensions/api_test/autotest_private/test.js [modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/extensions/browser/extension_function_histogram_value.h [modify] https://crrev.com/d127ade1e89dd1b221e85170e0dbe4c32072c7c5/tools/metrics/histograms/histograms.xml
,
Apr 27 2017
,
Apr 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/f2c3bdeb201a43f86a35fe40c3eb1b920c2c5bba commit f2c3bdeb201a43f86a35fe40c3eb1b920c2c5bba Author: khmel <khmel@google.com> Date: Thu Apr 27 22:02:20 2017 arc: Switch to using autotest API to enable Play Store Need to discard using settings based approach once old settings expires soon. BUG= chromium:694081 TEST=Manually, together with crrev.com/2801173002 Change-Id: I906340935fd2f4faa9fc7f6783925f6b89728050 Reviewed-on: https://chromium-review.googlesource.com/470707 Commit-Ready: Yury Khmel <khmel@chromium.org> Tested-by: Yury Khmel <khmel@chromium.org> Reviewed-by: Luis Hector Chavez <lhchavez@chromium.org> Reviewed-by: Yury Khmel <khmel@chromium.org> [modify] https://crrev.com/f2c3bdeb201a43f86a35fe40c3eb1b920c2c5bba/client/common_lib/cros/chrome.py [modify] https://crrev.com/f2c3bdeb201a43f86a35fe40c3eb1b920c2c5bba/client/common_lib/cros/arc_util.py
,
May 3 2017
,
May 23 2017
Issue 666871 has been merged into this issue.
,
Jan 22 2018
|
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by kinaba@chromium.org
, Feb 20 2017