More test tools for HTTP testing

Here are a set of classes (and patches) for abstracting the HTTP testing for the media player into several different test cases. This may be helpful in other testing.

Also included are updated test cases for corner cases although these sometimes don't all pass on 2.1 and earlier — I think there are still some bugs in the media player there that I'm trying to nail down. (e.g issue 2, 6 and possibly 26, 41).

[Michael Frederick, 2010-10-01 16:26:58]

http://codereview.appspot.com/2321041/diff/1/Npr/src/org/npr/android/news/Pla...
File Npr/src/org/npr/android/news/PlaybackService.java (right):

http://codereview.appspot.com/2321041/diff/1/Npr/src/org/npr/android/news/Pla...
Npr/src/org/npr/android/news/PlaybackService.java:199: if( proxy != null ) {
Check spacing, e.g.
if (proxy != null) {

http://codereview.appspot.com/2321041/diff/1/Npr/src/org/npr/android/news/Pla...
File Npr/src/org/npr/android/news/PlayerActivity.java (right):

http://codereview.appspot.com/2321041/diff/1/Npr/src/org/npr/android/news/Pla...
Npr/src/org/npr/android/news/PlayerActivity.java:104: // Don't log an error
here; we anticipate an error on SDK < 5
Good catch.

http://codereview.appspot.com/2321041/diff/1/Npr_Test/default.properties
File Npr_Test/default.properties (right):

http://codereview.appspot.com/2321041/diff/1/Npr_Test/default.properties#newc...
Npr_Test/default.properties:11: target=android-3
I've had problems setting the target to older versions. I haven't tried
specifically with the Test app. Is there anything in particular that this does?

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

http://codereview.appspot.com/2321041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/PlaybackServiceTest.java:175: * were getting
locked.
I think the old grammar was correct.

http://codereview.appspot.com/2321041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/PlaybackServiceTest.java:240:
"http://91.121.62.121:8080/",
Let's pick one value here, or write a separate test for each if we're testing
different things.

[jeremy.wadsack, 2010-10-01 18:17:02]

Updates as suggested and responses.

http://codereview.appspot.com/2321041/diff/1/Npr_Test/default.properties
File Npr_Test/default.properties (right):

http://codereview.appspot.com/2321041/diff/1/Npr_Test/default.properties#newc...
Npr_Test/default.properties:11: target=android-3
On 2010/10/01 16:26:59, Michael Frederick wrote:
> I've had problems setting the target to older versions. I haven't tried
> specifically with the Test app. Is there anything in particular that this
does?

I just wanted to make sure it matches the app.


"Unless you know that you'll be using new APIs introduced in the latest SDK, you
should select a target with the lowest platform version possible."
http://developer.android.com/guide/developing/eclipse-adt.html

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

http://codereview.appspot.com/2321041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/PlaybackServiceTest.java:175: * were getting
locked.
On 2010/10/01 16:26:59, Michael Frederick wrote:
> I think the old grammar was correct.
Clearly. ;)

http://codereview.appspot.com/2321041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/PlaybackServiceTest.java:240:
"http://91.121.62.121:8080/",
On 2010/10/01 16:26:59, Michael Frederick wrote:
> Let's pick one value here, or write a separate test for each if we're testing
> different things.

Ugh, sorry. I wanted to get this to you in case you could use the source. This
test shouldn't depend on any external stream — it should only use the local host
entry. I was just playing with external streams and this test to try some other
stuff. 

I'll clean that all up.

[jeremy.wadsack, 2010-10-04 23:34:48]

One more thought on the 'target' value. Could use some clarification here.

http://codereview.appspot.com/2321041/diff/1/Npr_Test/default.properties
File Npr_Test/default.properties (right):

http://codereview.appspot.com/2321041/diff/1/Npr_Test/default.properties#newc...
Npr_Test/default.properties:11: target=android-3
On 2010/10/01 18:17:03, jeremy.wadsack wrote:
> On 2010/10/01 16:26:59, Michael Frederick wrote:
> > I've had problems setting the target to older versions. I haven't tried
> > specifically with the Test app. Is there anything in particular that this
> does?
> 
> I just wanted to make sure it matches the app.
> 
> 
> "Unless you know that you'll be using new APIs introduced in the latest SDK,
you
> should select a target with the lowest platform version possible."
> http://developer.android.com/guide/developing/eclipse-adt.html

I think I might be confused on this. There's 'target' in the .project, which if
it doesn't match the minSDKVersion in the AndroidManifest.xml gives a warning in
Eclipse. That's why I changed this.

There's also targetSDKVersion in AndroidManifest which is probably what the
statement from the docs is referring to as that has to do with how the project
is built and where it's intended to run.

[Michael Frederick, 2010-10-05 00:13:28]

http://codereview.appspot.com/2321041/diff/1/Npr_Test/default.properties
File Npr_Test/default.properties (right):

http://codereview.appspot.com/2321041/diff/1/Npr_Test/default.properties#newc...
Npr_Test/default.properties:11: target=android-3
On 2010/10/04 23:34:49, jeremy.wadsack wrote:
> On 2010/10/01 18:17:03, jeremy.wadsack wrote:
> > On 2010/10/01 16:26:59, Michael Frederick wrote:
> > > I've had problems setting the target to older versions. I haven't tried
> > > specifically with the Test app. Is there anything in particular that this
> > does?
> > 
> > I just wanted to make sure it matches the app.
> > 
> > 
> > "Unless you know that you'll be using new APIs introduced in the latest SDK,
> you
> > should select a target with the lowest platform version possible."
> > http://developer.android.com/guide/developing/eclipse-adt.html
> 
> I think I might be confused on this. There's 'target' in the .project, which
if
> it doesn't match the minSDKVersion in the AndroidManifest.xml gives a warning
in
> Eclipse. That's why I changed this.
> 
> There's also targetSDKVersion in AndroidManifest which is probably what the
> statement from the docs is referring to as that has to do with how the project
> is built and where it's intended to run.

Yes, that's my understanding. I usually keep my SDK and target up to the latest
version. It's definitely confusing though, since some newer features are
backwards-compatible and others aren't. As long as you're not accessing newer
APIs, this seems like the best approach.

[jeremy.wadsack, 2010-10-05 00:19:49]

Ok. We'll leave that properties as it was. Anything else?

http://codereview.appspot.com/2321041/diff/1/Npr_Test/default.properties
File Npr_Test/default.properties (right):

http://codereview.appspot.com/2321041/diff/1/Npr_Test/default.properties#newc...
Npr_Test/default.properties:11: target=android-3
On 2010/10/05 00:13:29, Michael Frederick wrote:
> On 2010/10/04 23:34:49, jeremy.wadsack wrote:
> > On 2010/10/01 18:17:03, jeremy.wadsack wrote:
> > > On 2010/10/01 16:26:59, Michael Frederick wrote:
> > > > I've had problems setting the target to older versions. I haven't tried
> > > > specifically with the Test app. Is there anything in particular that
this
> > > does?
> > > 
> > > I just wanted to make sure it matches the app.
> > > 
> > > 
> > > "Unless you know that you'll be using new APIs introduced in the latest
SDK,
> > you
> > > should select a target with the lowest platform version possible."
> > > http://developer.android.com/guide/developing/eclipse-adt.html
> > 
> > I think I might be confused on this. There's 'target' in the .project, which
> if
> > it doesn't match the minSDKVersion in the AndroidManifest.xml gives a
warning
> in
> > Eclipse. That's why I changed this.
> > 
> > There's also targetSDKVersion in AndroidManifest which is probably what the
> > statement from the docs is referring to as that has to do with how the
project
> > is built and where it's intended to run.
> 
> Yes, that's my understanding. I usually keep my SDK and target up to the
latest
> version. It's definitely confusing though, since some newer features are
> backwards-compatible and others aren't. As long as you're not accessing newer
> APIs, this seems like the best approach.

Ok. I'll leave this out of the update then.

[Michael Frederick, 2010-10-05 00:41:07]

LGTM

http://codereview.appspot.com/2321041/diff/13001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/PlaybackService.java (right):

http://codereview.appspot.com/2321041/diff/13001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/PlaybackService.java:71: // This is set if the
currently playing item fails to play, so that
Can you fix this comment?