Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement createObservable for PreparedGetObject #568

Merged
merged 3 commits into from Dec 9, 2015
Merged

Implement createObservable for PreparedGetObject #568

merged 3 commits into from Dec 9, 2015

Conversation

geralt-encore
Copy link
Collaborator

I am not sure about my integration tests

TestSubscriber<User> testSubscriber = new TestSubscriber<User>();
userObservable.subscribe(testSubscriber);

testSubscriber.awaitTerminalEvent(500, TimeUnit.MILLISECONDS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static import for milliseconds please :)

@artem-zinnatullin
Copy link
Member

Looks good! Just few comments.

@artem-zinnatullin
Copy link
Member

@nikitin-da PTAL :)

@@ -42,6 +50,22 @@ public void shouldGetByQueryWithoutTypeMappingBlocking() {
}

@Test
public void shouldGetByQueryWithoutTypeMappingAsObservable() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe shouldGetObjectByQueryWithoutTypeMappingAsObservable will be better=)

@nikitin-da
Copy link
Collaborator

👍 LGTM

@artem-zinnatullin
Copy link
Member

Would be nice to also have integration tests to see that it reacts on changes of tables from query and emits new result!

@geralt-encore
Copy link
Collaborator Author

Ok, I will get my hands on it today in the evening

@artem-zinnatullin
Copy link
Member

Thanks!

@geralt-encore
Copy link
Collaborator Author

@artem-zinnatullin check this out!

}

@Test
public void getObjectBlockingWithRawQueryObservable() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneeded Blocking in test name

@artem-zinnatullin
Copy link
Member

Checked 👯

@geralt-encore
Copy link
Collaborator Author

But without this testSubscriber.awaitTerminalEvent(500, MILLISECONDS) my tests are failing. If I could somehow change Schedulers.io() to Schedulers.immediate() i would fix problem, but I couldn't find easy solution for it

@artem-zinnatullin
Copy link
Member

testSubscriber.awaitTerminalEvent is just an attempt to wait, tests can fail on slow computer.

Better to do take(1), etc to explicitly set expected number of items and then do awaitTerminalEvent() with huge timeout just to break the test if something gone wrong!

@geralt-encore
Copy link
Collaborator Author

Will it be something like that?

  @Test
public void queryOneExistedObjectObservable() {
    final List<User> users = putUsersBlocking(3);
    final User expectedUser = users.get(0);

    final Observable<User> userObservable = storIOSQLite
            .get()
            .object(User.class)
            .withQuery(Query.builder()
                    .table(UserTableMeta.TABLE)
                    .where(UserTableMeta.COLUMN_EMAIL + "=?")
                    .whereArgs(expectedUser.email())
                    .build())
            .prepare()
            .createObservable()
            .take(1);

    TestSubscriber<User> testSubscriber = new TestSubscriber<User>();
    userObservable.subscribe(testSubscriber);

    testSubscriber.awaitTerminalEvent(5, SECONDS);
    testSubscriber.assertValueCount(1);
    testSubscriber.assertValue(expectedUser);
    testSubscriber.assertNoErrors();
    testSubscriber.unsubscribe();
}

@artem-zinnatullin
Copy link
Member

Yep, looks good, you can remove assertValueCount() because assertValue() checks values count :)

@geralt-encore
Copy link
Collaborator Author

Tests are fixed now

testSubscriber.awaitTerminalEvent(5, SECONDS);
testSubscriber.assertValue(expectedUser);
testSubscriber.assertNoErrors();
testSubscriber.unsubscribe();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed but I think I'll merge it anyway :)

@artem-zinnatullin
Copy link
Member

👍 thanks again!

artem-zinnatullin added a commit that referenced this pull request Dec 9, 2015
Implement createObservable for PreparedGetObject
@artem-zinnatullin artem-zinnatullin merged commit ace7c59 into pushtorefresh:master Dec 9, 2015
@geralt-encore
Copy link
Collaborator Author

I'll try to finish Rx part for content resolver by the end of this week. Could you explain why calling unsubscribe for TestSubscriber is unnecessary? Sorry for a huge amount of defects in this PR =(

@artem-zinnatullin
Copy link
Member

@geralt-encore no problems! You help us a lot (I'm busy as always)! Also, you learned some Rx and that's great :)

@geralt-encore geralt-encore deleted the feature/object-observable branch December 17, 2015 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants