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

chore: add sync expression to multi-auth test #1506

Merged
merged 3 commits into from Sep 21, 2021
Merged

chore: add sync expression to multi-auth test #1506

merged 3 commits into from Sep 21, 2021

Conversation

rjuliano
Copy link
Contributor

Issue #, if available:

Description of changes:
Some of the multi-auth tests were failing sporadically likely due to the amount of data being sync'd. The sync expression should prevent that.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@rjuliano rjuliano requested a review from a team September 21, 2021 16:56
Copy link
Contributor

@raphkim raphkim left a comment

Choose a reason for hiding this comment

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

Few comments here and there, but looks good otherwise.

Comment on lines 818 to 823
.syncExpression(modelSchema.getName(), new DataStoreSyncExpression() {
@Override
public QueryPredicate resolvePredicate() {
return Where.id("FAKE_ID").getQueryPredicate();
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we use lambda here?

.syncExpression(modelSchema.getName(), () -> Where.id("FAKE_ID").getQueryPredicate())

Copy link
Contributor

Choose a reason for hiding this comment

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

Where.id() helper method also has a bug with ambiguous column names IIRC. I guess it's fine unless we start writing multiauth tests that has model relationship

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah...I think for the purposes of this test, we're using a single table for each test case so we should be OK

Comment on lines 813 to 817
.errorHandler(exception -> {
Log.e(tag,
"DataStore error handler received an error.",
exception);
})
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need {} for single liners :)

.errorHandler(error -> Log.e(tag, message, error))

but even better, wouldn't a Hub publish be more useful here for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it to be a lambda expression. I only added this in here to make sure we could pick up on any exceptions thrown during the test. We can explore some alternatives as part of a different PR though.

@raphkim raphkim merged commit 8b0695f into main Sep 21, 2021
@raphkim raphkim deleted the rjuliano/syncxp branch September 21, 2021 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants