Skip to content

Commit

Permalink
chromeos: Re-enable Android app parts of OSSettingsAppsPageTest
Browse files Browse the repository at this point in the history 
https://chromium-review.googlesource.com/c/chromium/src/+/1815957 merged
the Google Play Store settings into the top-level Apps section. This
removed the ANDROID_APPS route, so the existing tests can't provide
coverage.

This CL rewrites the tests of the Android apps sub-page so they
construct the sub-page directly, rather than trying to click to
navigate to it. This avoids the problem with the ANDROID_APPS route,
and may help with the flakiness that caused the tests to be disabled
in the first place.

Bug: 1004583, 1006662
Test: browser_tests
Change-Id: I9e22a1c4a90163b1c3093e5ec1648e62f7e4a61f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1824020
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Jeevan Shikaram <jshikaram@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699597}
  • Loading branch information
James Cook authored and Commit Bot committed Sep 25, 2019
1 parent 45d3c7f commit 10ae04f
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 132 deletions.
3 changes: 2 additions & 1 deletion chrome/browser/resources/settings/android_apps_page/android_apps_subpage.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
on-click="onManageAndroidAppsTap_" external></cr-link-row>
</template>

<template is="dom-if" if="[[allowRemove_(prefs.arc.enabled.*)]]">
<!-- Use 'restamp' so tests can check if the row exists. -->
<template is="dom-if" if="[[allowRemove_(prefs.arc.enabled.*)]]" restamp>
<div id="remove" class="settings-box">
<div id="androidRemoveLabel" class="start">
$i18n{androidAppsRemove}
Expand Down
4 changes: 0 additions & 4 deletions chrome/test/data/webui/settings/chromeos/android_apps_page_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ suite('AndroidAppsPageTests', function() {
androidAppsPage.remove();
});

teardown(function() {
androidAppsPage.remove();
});

suite('Main Page', function() {
setup(function() {
androidAppsPage.havePlayStoreApp = true;
Expand Down
170 changes: 68 additions & 102 deletions chrome/test/data/webui/settings/chromeos/apps_page_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,10 @@ let appsPage = null;
/** @type {?TestAndroidAppsBrowserProxy} */
let androidAppsBrowserProxy = null;

const setAndroidAppsState = function(playStoreEnabled, settingsAppAvailable) {
const appsInfo = {
playStoreEnabled: playStoreEnabled,
settingsAppAvailable: settingsAppAvailable,
};
appsPage.androidAppsInfo = appsInfo;
appsPage.showAndroidApps = true;
Polymer.dom.flush();
};

suite('AppsPageTests', function() {
setup(function() {
androidAppsBrowserProxy = new TestAndroidAppsBrowserProxy();
settings.AndroidAppsBrowserProxyImpl.instance_ = androidAppsBrowserProxy;
PolymerTest.clearBody();
appsPage = document.createElement('os-settings-apps-page');
document.body.appendChild(appsPage);
Expand All @@ -28,6 +20,7 @@ suite('AppsPageTests', function() {

teardown(function() {
appsPage.remove();
appsPage = null;
});

suite('Page Combinations', function() {
Expand Down Expand Up @@ -75,31 +68,20 @@ suite('AppsPageTests', function() {
assertTrue(AndroidAppsShown());
});
});
});

// Changes to this suite should be reflected in android_apps_page_test.js
suite('AndroidAppsDetailPageTests', function() {
setup(function() {
androidAppsBrowserProxy = new TestAndroidAppsBrowserProxy();
settings.AndroidAppsBrowserProxyImpl.instance_ = androidAppsBrowserProxy;
PolymerTest.clearBody();
appsPage = document.createElement('os-settings-apps-page');
document.body.appendChild(appsPage);
testing.Test.disableAnimationsAndTransitions();
});

teardown(function() {
appsPage.remove();
});

suite('Main Page', function() {
setup(function() {
appsPage.showAndroidApps = true;
appsPage.havePlayStoreApp = true;
appsPage.prefs = {arc: {enabled: {value: false}}};
setAndroidAppsState(false, false);
appsPage.androidAppsInfo = {
playStoreEnabled: false,
settingsAppAvailable: false,
};
Polymer.dom.flush();
});

test('Enable', function() {
test('Clicking enable button enables ARC', function() {
const button = appsPage.$$('#enable');
assertTrue(!!button);
assertFalse(!!appsPage.$$('.subpage-arrow'));
Expand All @@ -108,45 +90,40 @@ suite('AndroidAppsDetailPageTests', function() {
Polymer.dom.flush();
assertTrue(appsPage.prefs.arc.enabled.value);

setAndroidAppsState(true, false);
appsPage.androidAppsInfo = {
playStoreEnabled: true,
settingsAppAvailable: false,
};
Polymer.dom.flush();
assertTrue(!!appsPage.$$('.subpage-arrow'));
});

// TODO(crbug.com/1006662): Test that setting playStoreEnabled to false
// navigates back to the main apps section.
});

// TODO(crbug.com/1006662): Fix test suite.
suite.skip('SubPage', function() {
let subpage;

function flushAsync() {
Polymer.dom.flush();
return new Promise(resolve => {
appsPage.async(resolve);
});
}

/**
* Returns a new promise that resolves after a window 'popstate' event.
* @return {!Promise}
*/
function whenPopState() {
return new Promise(function(resolve) {
window.addEventListener('popstate', function callback() {
window.removeEventListener('popstate', callback);
resolve();
});
});
}
suite('Android apps subpage', function() {
let subpage = null;

setup(function() {
appsPage.havePlayStoreApp = true;
appsPage.prefs = {arc: {enabled: {value: true}}};
setAndroidAppsState(true, false);
settings.navigateTo(settings.routes.ANDROID_APPS);
appsPage.$$('#android-apps').click();
return flushAsync().then(() => {
subpage = appsPage.$$('settings-android-apps-subpage');
assertTrue(!!subpage);
});
androidAppsBrowserProxy = new TestAndroidAppsBrowserProxy();
settings.AndroidAppsBrowserProxyImpl.instance_ = androidAppsBrowserProxy;
PolymerTest.clearBody();
subpage = document.createElement('settings-android-apps-subpage');
document.body.appendChild(subpage);
testing.Test.disableAnimationsAndTransitions();

subpage.prefs = {arc: {enabled: {value: true}}};
subpage.androidAppsInfo = {
playStoreEnabled: true,
settingsAppAvailable: false,
};
Polymer.dom.flush();
});

teardown(function() {
subpage.remove();
subpage = null;
});

test('Sanity', function() {
Expand All @@ -156,14 +133,27 @@ suite('AndroidAppsDetailPageTests', function() {

test('ManageAppsUpdate', function() {
assertTrue(!subpage.$$('#manageApps'));
setAndroidAppsState(true, true);
subpage.androidAppsInfo = {
playStoreEnabled: true,
settingsAppAvailable: true,
};
Polymer.dom.flush();
assertTrue(!!subpage.$$('#manageApps'));
setAndroidAppsState(true, false);

subpage.androidAppsInfo = {
playStoreEnabled: true,
settingsAppAvailable: false,
};
Polymer.dom.flush();
assertTrue(!subpage.$$('#manageApps'));
});

test('ManageAppsOpenRequest', function() {
setAndroidAppsState(true, true);
subpage.androidAppsInfo = {
playStoreEnabled: true,
settingsAppAvailable: true,
};
Polymer.dom.flush();
const button = subpage.$$('#manageApps');
assertTrue(!!button);
const promise =
Expand All @@ -187,58 +177,34 @@ suite('AndroidAppsDetailPageTests', function() {
dialog.close();
});

test('HideOnDisable', function() {
assertEquals(
settings.getCurrentRoute(), settings.routes.ANDROID_APPS_DETAILS);
setAndroidAppsState(false, false);
return whenPopState().then(function() {
assertEquals(settings.getCurrentRoute(), settings.routes.ANDROID_APPS);
});
});
});

// TODO(crbug.com/1006662): Fix test suite.
suite.skip('Enforced', function() {
let subpage;

setup(function() {
appsPage.havePlayStoreApp = true;
appsPage.prefs = {
test('ARC enabled by policy', function() {
subpage.prefs = {
arc: {
enabled: {
value: true,
enforcement: chrome.settingsPrivate.Enforcement.ENFORCED
}
}
};
setAndroidAppsState(true, true);
assertTrue(!!settings.routes.ANDROID_APPS_DETAILS);
appsPage.$$('#android-apps').click();
subpage.androidAppsInfo = {
playStoreEnabled: true,
settingsAppAvailable: true,
};
Polymer.dom.flush();
subpage = appsPage.$$('settings-android-apps-subpage');
assertTrue(!!subpage);
});

test('Sanity', function(done) {
Polymer.dom.flush();
assertFalse(!!subpage.$$('#remove'));
assertTrue(!!subpage.$$('#manageApps'));
});
});

suite('NoPlayStore', function() {
setup(function() {
appsPage.havePlayStoreApp = false;
appsPage.prefs = {arc: {enabled: {value: true}}};
setAndroidAppsState(true, true);
});

test('Sanity', function() {
assertTrue(!!appsPage.$$('#manageApps'));
});
test('Can open app settings without Play Store', function() {
subpage.prefs = {arc: {enabled: {value: true}}};
subpage.androidAppsInfo = {
playStoreEnabled: false,
settingsAppAvailable: true,
};
Polymer.dom.flush();

test('ManageAppsOpenRequest', function() {
const button = appsPage.$$('#manageApps');
const button = subpage.$$('#manageApps');
assertTrue(!!button);
const promise =
androidAppsBrowserProxy.whenCalled('showAndroidAppsSettings');
Expand Down
26 changes: 1 addition & 25 deletions chrome/test/data/webui/settings/chromeos/os_settings_browsertest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,31 +116,7 @@ TEST_F('OSSettingsAdvancedPageBrowserTest', 'DISABLED_AllJsTests', () => {
mocha.run();
});

// Tests for the Android App section in Google Play Store.
// eslint-disable-next-line no-var
var OSSettingsAndroidAppsPageTest = class extends OSSettingsBrowserTest {
/** @override */
get browsePreload() {
return super.browsePreload + 'android_apps_page/android_apps_page.html';
}

/** @override */
get extraLibraries() {
return super.extraLibraries.concat([
'//ui/webui/resources/js/promise_resolver.js',
BROWSER_SETTINGS_PATH + '../test_browser_proxy.js',
BROWSER_SETTINGS_PATH + 'chromeos/test_android_apps_browser_proxy.js',
'android_apps_page_test.js',
]);
}
};

// Disabled due to flakiness on linux-chromeos-rel
TEST_F('OSSettingsAndroidAppsPageTest', 'DISABLED_AllJsTests', () => {
mocha.run();
});

// Tests for the Android App section in Google Play Store.
// Tests for the Apps section (combines Chrome and Android apps).
// eslint-disable-next-line no-var
var OSSettingsAppsPageTest = class extends OSSettingsBrowserTest {
/** @override */
Expand Down

0 comments on commit 10ae04f

Please sign in to comment.