Add a UrlManagerTest

Add a UrlManagerTest

BUG=529962
Committed: https://crrev.com/db9001f67795ae7297ce02fcf01e6f40889b2661
Cr-Commit-Position: refs/heads/master@{#367982}

[cco3, 2015-12-30 22:23:42]

Description was changed from

==========
Add a UrlManagerTest

BUG=529962
==========

to

==========
Add a UrlManagerTest

BUG=529962
==========

[cco3, 2016-01-04 19:28:14]

cco3@chromium.org changed reviewers:
+ newt@chromium.org

[newt (away), 2016-01-05 02:11:52]

Thanks for adding this test :)

Could you get someone on the physical web team to do the main review, and I'll
take a look after for the final review?

[conleyo, 2016-01-05 02:19:07]

conleyo@google.com changed reviewers:
+ conleyo@google.com, mattreynolds@chromium.org

[mattreynolds, 2016-01-05 19:27:21]

LGTM, though the MockPwsClient will need to be built out more when we add tests
for removeUrl and clearUrls

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java
(right):

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java:43:
void resolve(final Collection<String> broadcastUrls,
Can we remove 'final' in the interface?

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java
(right):

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java:29:
static final String PREF_PHYSICAL_WEB = "physical_web";
Nit: Can we make this and the PHYSICAL_WEB_* constants package-private in
PrivacyPreferencesManager to avoid duplication?

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java:90:
Set<String> urls = mUrlManager.getUrls();
This would be a convenient place to test that getUrls(true) returns unresolved
URLs when there are no resolved URLs

Set<String> urls = mUrlManager.getUrls(true);
assertEquals(1, urls.size());

[newt (away), 2016-01-05 19:49:09]

mostly rubberstamp lgtm after comment

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java
(right):

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java:25:
public class UrlManagerTest extends
BaseActivityInstrumentationTestCase<Activity> {
If you don't actually need an activity to run this test (which appears to be the
case), then just extend InstrumentationTestCase.

[cco3, 2016-01-06 20:02:15]

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java
(right):

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PwsClient.java:43:
void resolve(final Collection<String> broadcastUrls,
On 2016/01/05 19:27:21, mattreynolds wrote:
> Can we remove 'final' in the interface?

Done.

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java
(right):

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java:25:
public class UrlManagerTest extends
BaseActivityInstrumentationTestCase<Activity> {
On 2016/01/05 19:49:09, newt wrote:
> If you don't actually need an activity to run this test (which appears to be
the
> case), then just extend InstrumentationTestCase.

Done.

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java:29:
static final String PREF_PHYSICAL_WEB = "physical_web";
On 2016/01/05 19:27:21, mattreynolds wrote:
> Nit: Can we make this and the PHYSICAL_WEB_* constants package-private in
> PrivacyPreferencesManager to avoid duplication?

That's a different package.  We can move them to PhysicalWeb.java in a different
change.

https://codereview.chromium.org/1552743004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java:90:
Set<String> urls = mUrlManager.getUrls();
On 2016/01/05 19:27:21, mattreynolds wrote:
> This would be a convenient place to test that getUrls(true) returns unresolved
> URLs when there are no resolved URLs
> 
> Set<String> urls = mUrlManager.getUrls(true);
> assertEquals(1, urls.size());

Done.

[cco3, 2016-01-06 20:02:19]

The CQ bit was checked by cco3@chromium.org

[cco3, 2016-01-06 20:02:20]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org,
mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/1552743004/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2016-01-06 20:04:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1552743004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1552743004/100001

[commit-bot: I haz the power, 2016-01-06 21:49:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-06 21:49:21]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[cco3, 2016-01-06 22:18:26]

On 2016/01/06 21:49:21, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

NoSuchMethodError?  This works when I run it locally, and I made those methods
public, so I don't know what is wrong.

[cco3, 2016-01-06 22:32:22]

The CQ bit was checked by cco3@chromium.org

[cco3, 2016-01-06 22:32:23]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org,
mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/1552743004/#ps120001
(title: "Removed commented out code, made test methods package private")

[commit-bot: I haz the power, 2016-01-06 22:34:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1552743004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1552743004/120001

[newt (away), 2016-01-06 22:39:56]

On 2016/01/06 22:18:26, cco3 wrote:
> On 2016/01/06 21:49:21, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
> 
> NoSuchMethodError?  This works when I run it locally, and I made those methods
> public, so I don't know what is wrong.

This is an issue that only appears in release builds, due to how we use
ProGuard. ProGuard removes methods from the main APK that are unused (among
other optimizations). So, at runtime the test APK tries to access methods in the
main APK that no longer exist, and a NoSuchMethodError is thrown.

To workaround this, methods that are only used in test APKs should be marked
"@VisibleForTesting", which signals ProGuard to keep the method, even if it's
unused. (See "VisibleForTesting" in chrome/android/java/proguard.flags)

[cco3, 2016-01-06 22:59:55]

On 2016/01/06 22:39:56, newt wrote:
> On 2016/01/06 22:18:26, cco3 wrote:
> > On 2016/01/06 21:49:21, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
> > 
> > NoSuchMethodError?  This works when I run it locally, and I made those
methods
> > public, so I don't know what is wrong.
> 
> This is an issue that only appears in release builds, due to how we use
> ProGuard. ProGuard removes methods from the main APK that are unused (among
> other optimizations). So, at runtime the test APK tries to access methods in
the
> main APK that no longer exist, and a NoSuchMethodError is thrown.
> 
> To workaround this, methods that are only used in test APKs should be marked
> "@VisibleForTesting", which signals ProGuard to keep the method, even if it's
> unused. (See "VisibleForTesting" in chrome/android/java/proguard.flags)

That makes sense.

[cco3, 2016-01-06 22:59:58]

The CQ bit was checked by cco3@chromium.org

[cco3, 2016-01-06 22:59:58]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org,
mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/1552743004/#ps140001
(title: "Use VisibleForTesting")

[commit-bot: I haz the power, 2016-01-06 23:01:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1552743004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1552743004/140001

[cco3, 2016-01-07 00:49:14]

The CQ bit was checked by cco3@chromium.org

[cco3, 2016-01-07 00:49:15]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org,
mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/1552743004/#ps160001
(title: "More of the same")

[commit-bot: I haz the power, 2016-01-07 00:49:20]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[commit-bot: I haz the power, 2016-01-07 00:49:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1552743004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1552743004/160001

[commit-bot: I haz the power, 2016-01-07 01:44:54]

Description was changed from

==========
Add a UrlManagerTest

BUG=529962
==========

to

==========
Add a UrlManagerTest

BUG=529962
==========

[commit-bot: I haz the power, 2016-01-07 01:44:55]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-01-07 01:46:10]

Description was changed from

==========
Add a UrlManagerTest

BUG=529962
==========

to

==========
Add a UrlManagerTest

BUG=529962

Committed: https://crrev.com/db9001f67795ae7297ce02fcf01e6f40889b2661
Cr-Commit-Position: refs/heads/master@{#367982}
==========

[commit-bot: I haz the power, 2016-01-07 01:46:11]

Patchset 9 (id:??) landed as
https://crrev.com/db9001f67795ae7297ce02fcf01e6f40889b2661
Cr-Commit-Position: refs/heads/master@{#367982}