New issue
Advanced search Search tips

Issue 725822 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

LargeIconServiceTest.* should be crisper

Project Member Reported by jkrcal@chromium.org, May 24 2017

Issue description

Time for some cleanup in LargeIconServiceTest. Ideas:

pkotwicz@:
- Make a local test version of
GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() which takes a
struct. That way you only need to specify non default values

- Build |kExpectedServerUrlWithoutCheck| from a "default expected URL".

e.g.
const GURL kExpectedServerUrlWithoutCheck =
ReplaceQueryParameter(kDefaultExpectedServerUrl, "check_seen=true", "");

Of course, ReplaceQueryParameter() could be more complicated

mastiz@:
I feel that the most severe
duplication here is not about the test fixture (e.g. irrelevant
|desired_size_in_pixel|) but the redundant test expectations
(|kExpectedServerUrl|). Except perhaps minor and trivial improvements like
CreateTestSkBitmap() taking default values or returning gfx::Image directly
(CreateTestImage()).

I think what we're trying to do here is to verify specifically one URL
parameter, which is "check_seen". Hence, I'd suggest either using
testing::HasSubstr() or preferably introducing a custom matcher,
QueryParameterEq("check_seen", _). This could be combined with the adoption of
ON_CALL for StartOrQueueNetworkRequest(_, _, _, _).
 

Comment 1 by jkrcal@chromium.org, Feb 26 2018

Components: UI>Browser>History
Cc: jkrcal@chromium.org
Owner: ----
Dropping the bug as it does not align with any of my current projects.
Status: Available (was: Assigned)

Sign in to add a comment