Boundary testing on local station lookup

These are some tests I put together to test the station lookup features for Issue 24. However, with these tests in place, nothing is failing, so this doesn't resolve issue 24 but it does provide coverage.

Note that this includes a patch to Main that adds location listeners on startup. This is needed in testing so that calling getLastKnownLocation (in station lookup) doesn't return null. There's an extreme corner case where this could happen in real life (a user turns on the device, starts NPR and asks for local station without any other app looking at the location).

[Michael Frederick, 2010-09-23 17:02:31]

Fantastic! Mostly pedantic comments below.

Question: Do you use Eclipse? If so, would it be helpful for me to create a
project-specific code formatter that can be imported directly? Then, everyone
can just auto-format their code before review.

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
File Npr/src/org/npr/android/news/Main.java (right):

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:73: private static final int
MSG_CANCEL_LOCATION_LISTENERS = 2;
While you're here, can you add constants for the others? Also, I usually prefer
enums in this case, since you get the ordinal value for free, and it's more
readable.

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:92: public List<LocationListener>
locationListeners = new ArrayList<LocationListener>();
>80 (and a few other places)

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:256: /*
Javadocs should start with /**

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:285: lm.requestLocationUpdates(provider,
0, 0, listener);
From the API: 
"Background services should be careful about setting a sufficiently high minTime
so that the device doesn't consume too much power by keeping the GPS or wireless
radios on all the time. In particular, values under 60000ms are not
recommended."

I'm hoping that it updates only a single time before it's removed... Can you
write a test to validate that?

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:290: /*
/**

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:295: synchronized (locationListeners) {
The synchronization here is to ensure that two different LocationListener
updates aren't trying to alter this in simultaneous threads? Please add a
comment.

http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
File Npr_Test/src/org/npr/android/news/StationDetailsActivityTest.java (right):

http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StationDetailsActivityTest.java:54: private
static final String LONG_URL =
"http://example.com/Lorem/ipsum/dolor/sit/amet,/consectetur/adipiscing/elit./Nunc/congue/justo/a/ipsum/pretium/ultrices./Vestibulum/a/elit/quam,/at/rutrum/est./Aliquam/erat/volutpat./Nunc/consectetur/pulvinar/elit/et/ultrices./Ut/congue/fringilla/tempus./Pellentesque/et/orci/id/dui/fermentum/luctus./Etiam/mollis/rhoncus/ante,/nec/varius/ligula/dignissim/sit/amet./Sed/quis/neque/quis/tellus/dapibus/mollis./Quisque/vel/sagittis/ipsum./Sed/accumsan/tristique/ante/sed/mattis./Sed/suscipit,/eros/non/semper/sollicitudin,/felis/sapien/consectetur/purus,/sed/venenatis/diam/dolor/cursus/neque./Cum/sociis/natoque/penatibus/et/magnis/dis/parturient/montes,/nascetur/ridiculus/mus.";
It's annoying, but please line wrap these manually. I believe that most IDEs can
do this automagically.

http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
File Npr_Test/src/org/npr/android/news/StationSearchActivityTest.java (right):

http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StationSearchActivityTest.java:45:
searchByGroup = (RadioGroup)activity.findViewById(R.id.RadioGroup01);
Spaces after casts, e.g.
(Foo) bar.getFoo();

http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StationSearchActivityTest.java:74: private
void mockLocationProvider( String providerName, double latitude, double
longitude ) {
Extraneous spaces.

[jeremy.wadsack, 2010-09-23 17:07:50]

Michael -

I do use Eclipse and I created a format and uploaded it to the downloads
section. However, if it's not correct, I would love if you could create a
correct one.

--
Jeremy Wadsack


On Thu, Sep 23, 2010 at 10:02 AM, <mfrederick@google.com> wrote:

> Fantastic! Mostly pedantic comments below.
>
> Question: Do you use Eclipse? If so, would it be helpful for me to
> create a project-specific code formatter that can be imported directly?
> Then, everyone can just auto-format their code before review.
>
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
> File Npr/src/org/npr/android/news/Main.java (right):
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
> Npr/src/org/npr/android/news/Main.java:73: private static final int
> MSG_CANCEL_LOCATION_LISTENERS = 2;
> While you're here, can you add constants for the others? Also, I usually
> prefer enums in this case, since you get the ordinal value for free, and
> it's more readable.
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
> Npr/src/org/npr/android/news/Main.java:92: public List<LocationListener>
> locationListeners = new ArrayList<LocationListener>();
>
>> 80 (and a few other places)
>>
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
> Npr/src/org/npr/android/news/Main.java:256: /*
> Javadocs should start with /**
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
> Npr/src/org/npr/android/news/Main.java:285:
> lm.requestLocationUpdates(provider, 0, 0, listener);
> From the API:
> "Background services should be careful about setting a sufficiently high
> minTime so that the device doesn't consume too much power by keeping the
> GPS or wireless radios on all the time. In particular, values under
> 60000ms are not recommended."
>
> I'm hoping that it updates only a single time before it's removed... Can
> you write a test to validate that?
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
> Npr/src/org/npr/android/news/Main.java:290: /*
> /**
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
> Npr/src/org/npr/android/news/Main.java:295: synchronized
> (locationListeners) {
> The synchronization here is to ensure that two different
> LocationListener updates aren't trying to alter this in simultaneous
> threads? Please add a comment.
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
> File Npr_Test/src/org/npr/android/news/StationDetailsActivityTest.java
> (right):
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
> Npr_Test/src/org/npr/android/news/StationDetailsActivityTest.java:54:
> private static final String LONG_URL =
> "
>
http://example.com/Lorem/ipsum/dolor/sit/amet,/consectetur/adipiscing/elit./N...
> .";
> It's annoying, but please line wrap these manually. I believe that most
> IDEs can do this automagically.
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
> File Npr_Test/src/org/npr/android/news/StationSearchActivityTest.java
> (right):
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
> Npr_Test/src/org/npr/android/news/StationSearchActivityTest.java:45:
> searchByGroup = (RadioGroup)activity.findViewById(R.id.RadioGroup01);
> Spaces after casts, e.g.
> (Foo) bar.getFoo();
>
>
>
http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
> Npr_Test/src/org/npr/android/news/StationSearchActivityTest.java:74:
> private void mockLocationProvider( String providerName, double latitude,
> double longitude ) {
> Extraneous spaces.
>
>
> http://codereview.appspot.com/2199045/
>

[jeremy.wadsack, 2010-09-23 23:53:18]

Updated to reflect change requests, including formatting.

MainTest now tests to make sure that the service launches the listeners and
cancels them.

Note that in the third patch set I changed the minTime to 60000 milliseconds
(per recommendation in the SDK). This doesn't actually increase the test time at
all so this appears to be the time *between* updates and not time to the first
update; which is what you would expect.

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
File Npr/src/org/npr/android/news/Main.java (right):

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:73: private static final int
MSG_CANCEL_LOCATION_LISTENERS = 2;
On 2010/09/23 17:02:32, Michael Frederick wrote:
> While you're here, can you add constants for the others? Also, I usually
prefer
> enums in this case, since you get the ordinal value for free, and it's more
> readable.

Will do. I didn't want to change existing code too much, but wanted to present
an example. :)

I also prefer enums but apparently Java programmers generally don't like them
(which is the Android SDK full if int constants instead of enums?). In this case
though, using an enum might not gain us much as we'd have to cast it and int to
store it in the message and cast it from and int to check it.

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:256: /*
On 2010/09/23 17:02:32, Michael Frederick wrote:
> Javadocs should start with /**

I guess I was trying to make these not javadoc. I suppose it doesn't matter, I
can do that.

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:285: lm.requestLocationUpdates(provider,
0, 0, listener);
On 2010/09/23 17:02:32, Michael Frederick wrote:
> From the API: 
> "Background services should be careful about setting a sufficiently high
minTime
> so that the device doesn't consume too much power by keeping the GPS or
wireless
> radios on all the time. In particular, values under 60000ms are not
> recommended."
> 
> I'm hoping that it updates only a single time before it's removed... Can you
> write a test to validate that?

Right. The SDK isn't conducive to dependency injection (which is why my tests
aren't as good as they could be). I think I can write a test that verifies that
listeners are added and removed from the listener collection. Which will have to
suffice.

Also, I thought about setting this with a large value (like 36M) but I couldn't
tell from the SDK whether the minTime was applied before the first request or
only between requests.

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:295: synchronized (locationListeners) {
On 2010/09/23 17:02:32, Michael Frederick wrote:
> The synchronization here is to ensure that two different LocationListener
> updates aren't trying to alter this in simultaneous threads? Please add a
> comment.

Correct. I can comment that.

http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
File Npr_Test/src/org/npr/android/news/StationDetailsActivityTest.java (right):

http://codereview.appspot.com/2199045/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StationDetailsActivityTest.java:54: private
static final String LONG_URL =
"http://example.com/Lorem/ipsum/dolor/sit/amet,/consectetur/adipiscing/elit./Nunc/congue/justo/a/ipsum/pretium/ultrices./Vestibulum/a/elit/quam,/at/rutrum/est./Aliquam/erat/volutpat./Nunc/consectetur/pulvinar/elit/et/ultrices./Ut/congue/fringilla/tempus./Pellentesque/et/orci/id/dui/fermentum/luctus./Etiam/mollis/rhoncus/ante,/nec/varius/ligula/dignissim/sit/amet./Sed/quis/neque/quis/tellus/dapibus/mollis./Quisque/vel/sagittis/ipsum./Sed/accumsan/tristique/ante/sed/mattis./Sed/suscipit,/eros/non/semper/sollicitudin,/felis/sapien/consectetur/purus,/sed/venenatis/diam/dolor/cursus/neque./Cum/sociis/natoque/penatibus/et/magnis/dis/parturient/montes,/nascetur/ridiculus/mus.";
On 2010/09/23 17:02:32, Michael Frederick wrote:
> It's annoying, but please line wrap these manually. I believe that most IDEs
can
> do this automagically.

Really? I had intentionally left them not wrapped so that they wouldn't
interfere with readability of the code. You can ignore a really long line that's
only one line tall when reading. If I wrap it, then it takes up lots of visual
space. I suppose I can put them at the bottom of the file.

[Michael Frederick, 2010-09-24 16:04:59]

LGTM

Great work! Just a few nitpicks.

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
File Npr/src/org/npr/android/news/Main.java (right):

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:73: private static final int
MSG_CANCEL_LOCATION_LISTENERS = 2;
On 2010/09/23 23:53:18, jeremy.wadsack wrote:
> On 2010/09/23 17:02:32, Michael Frederick wrote:
> > While you're here, can you add constants for the others? Also, I usually
> prefer
> > enums in this case, since you get the ordinal value for free, and it's more
> > readable.
> 
> Will do. I didn't want to change existing code too much, but wanted to present
> an example. :)
> 
> I also prefer enums but apparently Java programmers generally don't like them
> (which is the Android SDK full if int constants instead of enums?). In this
case
> though, using an enum might not gain us much as we'd have to cast it and int
to
> store it in the message and cast it from and int to check it.
> 
> 

Enum#ordinal returns a unique int for that value.

http://codereview.appspot.com/2199045/diff/10001/Npr_Test/src/org/npr/android...
File Npr_Test/src/org/npr/android/news/MainTest.java (right):

http://codereview.appspot.com/2199045/diff/10001/Npr_Test/src/org/npr/android...
Npr_Test/src/org/npr/android/news/MainTest.java:69: .e(
Personally, I find this less readable (having the method on a separate line). Is
this personal style, or a code auto-format?

[jeremy.wadsack, 2010-09-24 23:52:02]

I'll make the format fix and commit this weekend or on Monday.

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
File Npr/src/org/npr/android/news/Main.java (right):

http://codereview.appspot.com/2199045/diff/1/Npr/src/org/npr/android/news/Mai...
Npr/src/org/npr/android/news/Main.java:73: private static final int
MSG_CANCEL_LOCATION_LISTENERS = 2;
On 2010/09/24 16:05:00, Michael Frederick wrote:
> On 2010/09/23 23:53:18, jeremy.wadsack wrote:
> > On 2010/09/23 17:02:32, Michael Frederick wrote:
> > > While you're here, can you add constants for the others? Also, I usually
> > prefer
> > > enums in this case, since you get the ordinal value for free, and it's
more
> > > readable.
> > 
> > Will do. I didn't want to change existing code too much, but wanted to
present
> > an example. :)
> > 
> > I also prefer enums but apparently Java programmers generally don't like
them
> > (which is the Android SDK full if int constants instead of enums?). In this
> case
> > though, using an enum might not gain us much as we'd have to cast it and int
> to
> > store it in the message and cast it from and int to check it.
> > 
> > 
> 
> Enum#ordinal returns a unique int for that value.

But that's a method call and a case expression must be a constant expression. To
do that I would need to change the switch to a series of if/else statements. I'm
not sure we're gaining anything.

http://codereview.appspot.com/2199045/diff/10001/Npr_Test/src/org/npr/android...
File Npr_Test/src/org/npr/android/news/MainTest.java (right):

http://codereview.appspot.com/2199045/diff/10001/Npr_Test/src/org/npr/android...
Npr_Test/src/org/npr/android/news/MainTest.java:69: .e(
On 2010/09/24 16:05:00, Michael Frederick wrote:
> Personally, I find this less readable (having the method on a separate line).
Is
> this personal style, or a code auto-format?

Ugh, yes. That's bad. It's something in the formatter in Eclipse. I'll fix it
before I commit. I'd love a new formatter that works exactly right. :)