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

Fix: DataStore E2E Integration Tests #596

Merged
merged 2 commits into from
Jun 29, 2020
Merged

Conversation

ruiguoamz
Copy link
Contributor

@ruiguoamz ruiguoamz commented Jun 29, 2020

Description of changes:
Added a filter inside testCreateMutateDelete(), testCreateThenMutateWithCondition() and testCreateThenMutateWithConditionFailOnSync() to make sure these three tests only care about related payload receiving from hub listener.

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

@ruiguoamz ruiguoamz added the datastore Issues related to the DataStore category label Jun 29, 2020
@ruiguoamz ruiguoamz requested review from palpatim and lawmicha June 29, 2020 20:30
@@ -45,8 +45,10 @@ class DataStoreEndToEndTests: SyncEngineIntegrationTestBase {
return
}

if post.id != newPost.id { return }
Copy link
Contributor

Choose a reason for hiding this comment

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

let post = try? mutationEvent.decodeModel() as? Post above at line 42 will fail the test if there is an existing mutation in the queue and is not a post. if we are enforcing that the hub listener only reacts to data for this test then it should include decoding the right model as well, ie.

guard if let post = try? mutationEvent.decodeModel() as? Post, post.id != newPost.id else {
   return 
}
```

Copy link
Contributor Author

@ruiguoamz ruiguoamz Jun 29, 2020

Choose a reason for hiding this comment

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

Something like this?

guard let mutationEvent = payload.data as? MutationEvent else {
      XCTFail("Can't cast payload as mutation event")
      return
}

guard let post = try? mutationEvent.decodeModel() as? Post, post.id != newPost.id else { return }

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the intent of this proposal, but a couple of things:

  1. You need to fix the id equality check:

    guard let post = try? mutationEvent.decodeModel() as? Post, post.id == newPost.id else {
        return
    }

    Otherwise you'll always be guaranteed to check the wrong post.

  2. Please add a comment explaining that this check is to protect against stray events being processed after the test has completed, and that it shouldn't be construed as a pattern necessary for production applications.

@ruiguoamz ruiguoamz merged commit 9fdb722 into main Jun 29, 2020
@ruiguoamz ruiguoamz deleted the datastore/e2eintegrationfix branch June 29, 2020 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datastore Issues related to the DataStore category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants