Skip to content

Commit

Permalink
[TTS][RSearches] Add tests for Related Searches
Browse files Browse the repository at this point in the history 
Adds some tests for Related Searches and do minor test refactoring to
expose more internal data and allow the Policy to override the
user-permission to send the URL (which is currently required by Related
Searches).

Adds some tests that Related Searches requests are being made and
results are coming back (from the fake server) when appropriate.

Some refactoring of the Network Communicator was needed to expose the
context to the fake server and to make sure all the functionality except
logging is done in the manager so tests will have those effects too.

BUG=1062737

Change-Id: Iecf2129785febdae8af026c5c3d12c89b12cfb46
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2689681
Commit-Queue: Donn Denman <donnd@chromium.org>
Reviewed-by: Sinan Sahin <sinansahin@google.com>
Cr-Commit-Position: refs/heads/master@{#857259}
  • Loading branch information
Donn Denman authored and Chromium LUCI CQ committed Feb 24, 2021
1 parent ae74076 commit 3ae821b
Show file tree
Hide file tree
Showing 7 changed files with 110 additions and 30 deletions.
10 changes: 8 additions & 2 deletions ...ndroid/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ public abstract class ContextualSearchContext {
@NonNull
private String mFluentLanguages = "";

// Whether the Related Searches functionality should also be activated.
private boolean mDoRelatedSearches;
// The Related Searches stamp - non-empty when Related Searches are being requested.
private String mRelatedSearchesStamp;

/** A {@link ContextualSearchContext} that ignores changes to the selection. */
static class ChangeIgnoringContext extends ContextualSearchContext {
Expand Down Expand Up @@ -281,6 +281,7 @@ boolean canResolve() {
* be requested.
*/
void prepareToResolve(boolean isExactSearch, String relatedSearchesStamp) {
mRelatedSearchesStamp = relatedSearchesStamp;
ContextualSearchContextJni.get().prepareToResolve(
mNativePointer, this, isExactSearch, relatedSearchesStamp);
}
Expand Down Expand Up @@ -579,6 +580,11 @@ long getPreviousEventId() {
return mPreviousEventId;
}

@VisibleForTesting
String getRelatedSearchesStamp() {
return mRelatedSearchesStamp;
}

// ============================================================================================
// Native callback support.
// ============================================================================================
Expand Down
33 changes: 19 additions & 14 deletions ...ndroid/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -529,18 +529,11 @@ private void showContextualSearch(@StateChangeReason int stateChangeReason) {
}

@Override
public void startSearchTermResolutionRequest(String selection, boolean isExactResolve) {
WebContents baseWebContents = getBaseWebContents();
if (baseWebContents != null && mContext != null && mContext.canResolve()) {
mContext.prepareToResolve(
isExactResolve, mPolicy.getRelatedSearchesStamp((getBasePageLanguage())));
ContextualSearchManagerJni.get().startSearchTermResolutionRequest(
mNativeContextualSearchManagerPtr, this, mContext, getBaseWebContents());
ContextualSearchUma.logResolveRequested(mSelectionController.isTapSelection());
} else {
// Something went wrong and we couldn't resolve.
hideContextualSearch(StateChangeReason.UNKNOWN);
}
public void startSearchTermResolutionRequest(
String selection, boolean isExactResolve, ContextualSearchContext searchContext) {
ContextualSearchManagerJni.get().startSearchTermResolutionRequest(
mNativeContextualSearchManagerPtr, this, mContext, getBaseWebContents());
ContextualSearchUma.logResolveRequested(mSelectionController.isTapSelection());
}

@Override
Expand Down Expand Up @@ -1747,8 +1740,20 @@ public void resolveSearchTerm() {

String selection = mSelectionController.getSelectedText();
assert !TextUtils.isEmpty(selection);
mNetworkCommunicator.startSearchTermResolutionRequest(
selection, mSelectionController.isAdjustedSelection());

WebContents baseWebContents = getBaseWebContents();
boolean isExactResolve = mSelectionController.isAdjustedSelection();
if (baseWebContents != null && mContext != null && mContext.canResolve()) {
mContext.prepareToResolve(
isExactResolve, mPolicy.getRelatedSearchesStamp(getBasePageLanguage()));
mNetworkCommunicator.startSearchTermResolutionRequest(
selection, mSelectionController.isAdjustedSelection(), mContext);
} else {
// Something went wrong and we couldn't resolve.
hideContextualSearch(StateChangeReason.UNKNOWN);
return;
}

// If the we were unable to start the resolve, we've hidden the UI and set the
// context to null.
if (mContext == null || mSearchPanel == null) return;
Expand Down
4 changes: 3 additions & 1 deletion ...src/org/chromium/chrome/browser/contextualsearch/ContextualSearchNetworkCommunicator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ interface ContextualSearchNetworkCommunicator {
* @param selection the current selected text.
* @param isExactResolve Whether the resolution should be restricted to an exact match with
* the given selection that cannot be expanded based on the response.
* @param searchContext The {@link ContextualSearchContext} that the search will use.
*/
void startSearchTermResolutionRequest(String selection, boolean isExactResolve);
void startSearchTermResolutionRequest(
String selection, boolean isExactResolve, ContextualSearchContext searchContext);

/**
* Handles a Search Term Resolution response.
Expand Down
14 changes: 14 additions & 0 deletions ...android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ class ContextualSearchPolicy {
private boolean mDidOverrideDecidedStateForTesting;
private boolean mDecidedStateForTesting;
private Integer mTapTriggeredPromoLimitForTesting;
private boolean mDidOverrideAllowSendingPageUrlForTesting;
private boolean mAllowSendingPageUrlForTesting;

/**
* Tracks whether the In-Panel-Help has been shown.
Expand Down Expand Up @@ -422,6 +424,8 @@ boolean doSendBasePageUrl() {
* @return Whether we can send a URL.
*/
private boolean hasSendUrlPermissions() {
if (mDidOverrideAllowSendingPageUrlForTesting) return mAllowSendingPageUrlForTesting;

// Check whether the user has enabled anonymous URL-keyed data collection.
// This is surfaced on the relatively new "Make searches and browsing better" user setting.
// In case an experiment is active for the legacy UI call through the unified consent
Expand Down Expand Up @@ -487,6 +491,16 @@ void overrideDecidedStateForTesting(boolean decidedState) {
mDecidedStateForTesting = decidedState;
}

/**
* Overrides the user preference for sending the page URL to Google.
* @param doAllowSendingPageUrl Whether to allow sending the page URL or not, for tests.
*/
@VisibleForTesting
void overrideAllowSendingPageUrlForTesting(boolean doAllowSendingPageUrl) {
mDidOverrideAllowSendingPageUrlForTesting = true;
mAllowSendingPageUrlForTesting = doAllowSendingPageUrl;
}

/**
* @return count of times the panel with the promo has been opened.
*/
Expand Down
21 changes: 18 additions & 3 deletions ...avatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class ContextualSearchFakeServer
private String mSearchTermRequested;
private boolean mIsOnline = true;
private boolean mIsExactResolve;
private ContextualSearchContext mSearchContext;

private boolean mDidEverCallWebContentsOnShow;

Expand Down Expand Up @@ -488,6 +489,7 @@ void reset() {
mLoadedUrlCount = 0;
mUseInvalidLowPriorityPath = false;
mIsExactResolve = false;
mSearchContext = null;
}

/**
Expand All @@ -511,6 +513,11 @@ boolean getIsExactResolve() {
return mIsExactResolve;
}

@VisibleForTesting
ContextualSearchContext getSearchContext() {
return mSearchContext;
}

//============================================================================================
// History Removal Helpers
//============================================================================================
Expand All @@ -528,10 +535,12 @@ public boolean hasRemovedUrl(String url) {
//============================================================================================

@Override
public void startSearchTermResolutionRequest(String selection, boolean isExactResolve) {
public void startSearchTermResolutionRequest(
String selection, boolean isExactResolve, ContextualSearchContext searchContext) {
mLoadedUrl = null;
mSearchTermRequested = selection;
mIsExactResolve = isExactResolve;
mSearchContext = searchContext;

if (mActiveResolveSearch != null) {
mActiveResolveSearch.notifySearchTermResolutionStarted();
Expand Down Expand Up @@ -582,7 +591,6 @@ public void registerFakeSearches() {
registerFakeNonResolveSearch(new FakeNonResolveSearch("term", "Term"));
registerFakeNonResolveSearch(new FakeNonResolveSearch("resolution", "Resolution"));

registerFakeResolveSearch(new FakeResolveSearch("intelligence", "Intelligence"));
registerFakeResolveSearch(new FakeResolveSearch("states", "States"));
// registerFakeResolveSearch(new FakeResolveSearch("states-near""StatesNear"));
registerFakeResolveSearch(new FakeResolveSearch("search", "Search"));
Expand All @@ -596,7 +604,14 @@ public void registerFakeSearches() {
FakeResolveSearch germanFakeTapSearch = new FakeResolveSearch("german", germanSearchTerm);
registerFakeResolveSearch(germanFakeTapSearch);

registerFakeResolveSearch(new FakeResolveSearch("intelligence", "Intelligence"));
// Setup the "intelligence" node to return Related Searches along with the usual result.
ResolvedSearchTerm intelligenceWithRelatedSearches =
new ResolvedSearchTerm.Builder(false, 200, "Intelligence", "Intelligence")
.setRelatedSearches(new String[] {"Related Search 1", "Related Search 2"})
.build();
FakeResolveSearch fakeSearchWithRelatedSearches =
new FakeResolveSearch("intelligence", intelligenceWithRelatedSearches);
registerFakeResolveSearch(fakeSearchWithRelatedSearches);

// Register a fake tap search that will fake a logged event ID from the server, when
// a fake tap is done on the intelligence-logged-event-id element in the test file.
Expand Down
55 changes: 46 additions & 9 deletions ...vatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchImageControl;
import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchPanel;
import org.chromium.chrome.browser.compositor.bottombar.contextualsearch.ContextualSearchQuickActionControl;
import org.chromium.chrome.browser.contextualsearch.ContextualSearchFakeServer.FakeResolveSearch;
import org.chromium.chrome.browser.contextualsearch.ContextualSearchFakeServer.FakeSlowResolveSearch;
import org.chromium.chrome.browser.contextualsearch.ContextualSearchInternalStateController.InternalState;
import org.chromium.chrome.browser.contextualsearch.ResolvedSearchTerm.CardTag;
Expand Down Expand Up @@ -185,6 +186,10 @@ public Iterable<ParameterSet> getParameters() {
private static final String LOW_PRIORITY_INVALID_SEARCH_ENDPOINT = "/s/invalid";
private static final String CONTEXTUAL_SEARCH_PREFETCH_PARAM = "&pf=c";

/**
* Feature maps that we use for parameterized tests.
*/

/** This represents the current fully-launched configuration. */
private static final ImmutableMap<String, Boolean> ENABLE_NONE =
ImmutableMap.of(ChromeFeatureList.CONTEXTUAL_SEARCH_LONGPRESS_RESOLVE, false,
Expand All @@ -201,6 +206,12 @@ public Iterable<ParameterSet> getParameters() {
ChromeFeatureList.CONTEXTUAL_SEARCH_LITERAL_SEARCH_TAP, true,
ChromeFeatureList.CONTEXTUAL_SEARCH_TRANSLATIONS, true);

/** Feature maps that we use for individual tests. */
private static final ImmutableMap<String, Boolean> ENABLE_RELATED_SEARCHES = ImmutableMap.of(
ChromeFeatureList.RELATED_SEARCHES, true, ChromeFeatureList.RELATED_SEARCHES_UI, false);
private static final ImmutableMap<String, Boolean> ENABLE_RELATED_SEARCHES_UI = ImmutableMap.of(
ChromeFeatureList.RELATED_SEARCHES, true, ChromeFeatureList.RELATED_SEARCHES_UI, true);

private ActivityMonitor mActivityMonitor;
private ContextualSearchFakeServer mFakeServer;
private ContextualSearchManager mManager;
Expand Down Expand Up @@ -521,8 +532,7 @@ public void waitForSelectionToBe(final String text) {
* Waits for the Search Term Resolution to become ready.
* @param search A given FakeResolveSearch.
*/
public void waitForSearchTermResolutionToStart(
final ContextualSearchFakeServer.FakeResolveSearch search) {
public void waitForSearchTermResolutionToStart(final FakeResolveSearch search) {
CriteriaHelper.pollInstrumentationThread(() -> {
return search.didStartSearchTermResolution();
}, "Fake Search Term Resolution never started.", TEST_TIMEOUT, DEFAULT_POLLING_INTERVAL);
Expand All @@ -532,8 +542,7 @@ public void waitForSearchTermResolutionToStart(
* Waits for the Search Term Resolution to finish.
* @param search A given FakeResolveSearch.
*/
public void waitForSearchTermResolutionToFinish(
final ContextualSearchFakeServer.FakeResolveSearch search) {
public void waitForSearchTermResolutionToFinish(final FakeResolveSearch search) {
CriteriaHelper.pollInstrumentationThread(() -> {
return search.didFinishSearchTermResolution();
}, "Fake Search was never ready.", TEST_TIMEOUT, DEFAULT_POLLING_INTERVAL);
Expand Down Expand Up @@ -586,9 +595,9 @@ private void simulateNonResolveSearch(String nodeId)
* @throws InterruptedException
* @throws TimeoutException
*/
private void simulateResolveSearch(String nodeId)
private FakeResolveSearch simulateResolveSearch(String nodeId)
throws InterruptedException, TimeoutException {
simulateResolvableSearchAndAssertResolveAndPreload(nodeId, true);
return simulateResolvableSearchAndAssertResolveAndPreload(nodeId, true);
}

/**
Expand All @@ -600,10 +609,9 @@ private void simulateResolveSearch(String nodeId)
* @throws InterruptedException
* @throws TimeoutException
*/
private void simulateResolvableSearchAndAssertResolveAndPreload(String nodeId,
private FakeResolveSearch simulateResolvableSearchAndAssertResolveAndPreload(String nodeId,
boolean isResolveExpected) throws InterruptedException, TimeoutException {
ContextualSearchFakeServer.FakeResolveSearch search =
mFakeServer.getFakeResolveSearch(nodeId);
FakeResolveSearch search = mFakeServer.getFakeResolveSearch(nodeId);
assertNotNull("Could not find FakeResolveSearch for node ID:" + nodeId, search);
search.simulate();
waitForPanelToPeek();
Expand All @@ -614,6 +622,7 @@ private void simulateResolvableSearchAndAssertResolveAndPreload(String nodeId,
assertNoSearchesLoaded();
assertNoWebContents();
}
return search;
}

/**
Expand Down Expand Up @@ -3699,4 +3708,32 @@ public void testLongpressResolveEnabled() throws Exception {
Assert.assertEquals(2, userActionMonitor.get("ContextualSearch.ManualRefine"));
Assert.assertEquals(2, userActionMonitor.get("ContextualSearch.SelectionEstablished"));
}

// --------------------------------------------------------------------------------------------
// Related Searches Feature tests: base feature enables requests, UI feature allows results.
// --------------------------------------------------------------------------------------------

@Test
@SmallTest
@Feature({"ContextualSearch"})
public void testRelatedSearchesRequestedWhenEnabled() throws Exception {
FeatureList.setTestFeatures(ENABLE_RELATED_SEARCHES);
mPolicy.overrideAllowSendingPageUrlForTesting(true);
simulateResolveSearch("search");
Assert.assertFalse("Related Searches should have been requested but were not!",
mFakeServer.getSearchContext().getRelatedSearchesStamp().isEmpty());
}

@Test
@SmallTest
@Feature({"ContextualSearch"})
public void testRelatedSearchesResponseWhenEnabled() throws Exception {
FeatureList.setTestFeatures(ENABLE_RELATED_SEARCHES_UI);
mFakeServer.reset();
FakeResolveSearch fakeSearch = simulateResolveSearch("intelligence");
ResolvedSearchTerm resolvedSearchTerm = fakeSearch.getResolvedSearchTerm();
Assert.assertTrue("Related Searches results should have been returned but were not!",
resolvedSearchTerm.relatedSearches().length > 0);
// TODO(donnd): Add a check that the searches appeared in the Panel once the Panel can.
}
}
3 changes: 2 additions & 1 deletion ...atests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchTapEventTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ public ContextualSearchManagerWrapper(ChromeActivity activity) {
}

@Override
public void startSearchTermResolutionRequest(String selection, boolean isExactResolve) {
public void startSearchTermResolutionRequest(
String selection, boolean isExactResolve, ContextualSearchContext searchContext) {
// Skip native calls and immediately "resolve" the search term.
onSearchTermResolutionResponse(true, 200, selection, selection, "", "", false, 0, 10,
"", "", "", "", QuickActionCategory.NONE, 0, "", "", 0, new String[0]);
Expand Down

0 comments on commit 3ae821b

Please sign in to comment.