Fix #21: connect and disconnect on the background executor #22
Conversation
…utor TubeSock already uses a background thread for `open()`, but it keeps the `close()` call on the calling thread. So `open()` is safe on the main thread, but `close()` is not.
|
I'll look into that. I'm not sure what can be tested other than ensuring it doesn't run on the UI thread -- but I'm pretty sure the tests are intentionally swapping the executor with a blocking, same-thread executor, so it might be a little difficult to test that unless we make a different mock that uses a different thread. |
|
I wrote a quick |
| }, executor); | ||
|
|
||
| reconnect(); | ||
| executor.pause(); |
There was a problem hiding this comment.
This is basically https://github.com/ParsePlatform/Parse-SDK-Android/blob/master/Parse/src/test/java/com/parse/TaskQueueTestHelper.java#L24-L59 right?
Can you just queue/dequeue instead of needing to have pause logic?
There was a problem hiding this comment.
The intent of pause/unpause (including their names) is inspired by the Robolectric scheduler mocks, which allow pausing (must manually advance), unpausing (runs everything blocking), and advancing a paused scheduler one task at a time.
In this case, the TaskQueueTestHelper would not work for us here, because the LiveQuery client expects an Executor, not a TaskQueue.
And the TaskQueueTestHelper actually is pausing and unpausing the TaskQueue (the javadoc even explicitly says "pauses the TaskQueue") -- so I'm not sure that I agree with the naming of that class's methods. "enqueue" and "dequeue" does not imply in any way that it will block or pause the TaskQueue: "Enqueue" means that it is adding a task to the queue, nothing else.
If the ParseLiveQueryClientImpl were actually using a TaskQueue, then we could make use of that class; but it doesn't use TaskQueue at all.
I think for the purposes of a text fixture, it really doesn't matter what it's called. My vote would be for pause and unpause… but I'm happy to rename the method if you think that would be a point of confusion?
There was a problem hiding this comment.
Yeah just wondering...anyways thanks for looking at it.
The end result will be the same for all the other tests, since it will only queue up tasks while it is paused, and it is unpaused by default. To control when the tasks run, the test should first pause the executor before the tasks get enqueued.
|
@rogerhu quick question: I see that the Travis job is configured to run |
|
Ahh, I see now: it's failing with For some reason the Travis console log doesn't show that failure, but the "raw log" does: https://travis-ci.org/ParsePlatform/ParseLiveQuery-Android/builds/214437295 |
|
Yeah we need to add our credentials to Travis (NEXUS_USERNAME/NEXUS_PASSWORD). http://stackoverflow.com/questions/12343452/how-to-publish-artifacts-in-travis-ci I opened a ticket with Sonatype to get push access, can add it I believe...unless someone else wants to beat me to it. |
|
Got it. I opened #26 to track this. I don't know anything about how Travis is setup or configured, so I can't help. |
|
Well I have access to repo. Let me try to add them and retry after.
…On Mar 23, 2017 18:20, "Joe" ***@***.***> wrote:
Got it. I opened #26
<#26> to
track this. I don't know anything about how Travis is setup or configured,
so I can't help.
Thanks
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#22 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACclKUbQzOtbzz6wMm0u7RC5GT7_SYO1ks5rovA0gaJpZM4MnM1v>
.
|
|
I relaunched one of the master merged and the snapshot seem to be released: |
TubeSock already uses a background thread for
open(), but it keeps theclose()call on the calling thread. Soopen()is safe on the main thread,but
close()is not.