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(datastore): Enable cloud sync in integration tests #1351

Merged
merged 3 commits into from
Mar 25, 2022

Conversation

HuiSF
Copy link
Member

@HuiSF HuiSF commented Feb 10, 2022

Issue #, if available:

Description of changes:

  • Forward native subscritpionDataProcessed (Android) and syncReceived (iOS) events to Dart DataStore hub event stream, these two events are named as subscriptionDataProcessed in dart implementation
  • Updated test suites to run with cloud sync
  • Split 23 testing models into smaller groups, and run flutter test separately to avoid long subscription and initial sync time which made tests run unstable in Android.

NOTE: Android integration tests stability depends on PR #1339 ...
NOTE2: Cloud sync is enabled for model relationship tests and basic model operation within this PR. More tests with cloud sync will be add separately.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@HuiSF HuiSF requested a review from a team as a code owner February 10, 2022 17:21
@ragingsquirrel3 ragingsquirrel3 self-requested a review February 10, 2022 23:25
Copy link
Contributor

@dnys1 dnys1 left a comment

Choose a reason for hiding this comment

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

There is a lot of duplicated code in the tests - is there anyway to DRY this up a bit?

Copy link
Contributor

@ragingsquirrel3 ragingsquirrel3 left a comment

Choose a reason for hiding this comment

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

Nice job with this integ test change. It seems pretty stable for me running locally. There might be some challenges with this in CI, but not in scope for this PR IMO. A few high-level callouts:

  1. Do we want the subscription events to be separate PR? If not, please reflect both changes in commit message when merged to it's there for anyone who is debugging/bisecting commits.
  2. Like Dillon said, would be nice to DRY up the testing logic, left a suggestion for that, but feel free to chat offline more.
  3. Test script should return non-zero status if anything fails.
  4. Mostly nits after that.

@@ -19,9 +19,9 @@ part 'HubEventElementWithMetadata.dart';

/// The model associated with a DataStore `outboxMutationEnqueued` or
/// `outboxMutationProcessed` Hub event.
class HubEventElement {
class HubEventElement<M extends Model> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@HuiSF
Copy link
Member Author

HuiSF commented Feb 17, 2022

  1. Do we want the subscription events to be separate PR? If not, please reflect both changes in commit message when merged to it's there for anyone who is debugging/bisecting commits.

Could to a merge instead of squash merge on this PR (I'll squash comment resolving commits before merge).

@HuiSF HuiSF force-pushed the chore/ds-integration-tests-cloud branch 2 times, most recently from b2cee98 to a73398c Compare March 16, 2022 15:35
@HuiSF HuiSF requested review from ragingsquirrel3 and dnys1 March 16, 2022 15:40
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2022

Codecov Report

Merging #1351 (c5d7078) into main (1b5af9d) will decrease coverage by 0.06%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##             main    #1351      +/-   ##
==========================================
- Coverage   46.19%   46.12%   -0.07%     
==========================================
  Files         256      257       +1     
  Lines       10086    10089       +3     
==========================================
- Hits         4659     4654       -5     
- Misses       5427     5435       +8     
Flag Coverage Δ
android-unit-tests ∅ <ø> (∅)
flutter-unit-tests 36.62% <68.96%> (-0.08%) ⬇️
ios-unit-tests 85.36% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...mplify_datastore/lib/method_channel_datastore.dart 67.66% <ø> (ø)
...n_interface/lib/src/types/hub/HubEventElement.dart 0.00% <0.00%> (ø)
...lib/src/types/hub/HubEventElementWithMetadata.dart 0.00% <ø> (ø)
.../src/types/hub/SubscriptionDataProcessedEvent.dart 0.00% <0.00%> (ø)
...store/lib/amplify_datastore_stream_controller.dart 85.00% <80.00%> (-12.50%) ⬇️
...it_tests/DataStoreHubEventStreamHandlerTests.swift 98.58% <100.00%> (ø)

@ragingsquirrel3
Copy link
Contributor

I think the datastore provision script needs a little fix to work with cloud sync, at https://github.com/aws-amplify/amplify-flutter/blob/main/packages/amplify_datastore/example/tool/add_api_request.json#L11 add:

    "conflictResolution": {
      "defaultResolutionStrategy": {
        "type": "AUTOMERGE"
      }
    }

Copy link
Contributor

@ragingsquirrel3 ragingsquirrel3 left a comment

Choose a reason for hiding this comment

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

A few small fixes needed and a nit, otherwise looks great! This improved so much, thanks for chipping away at a big, complicated problem and leaving us with this great script.

💯

@HuiSF HuiSF force-pushed the chore/ds-integration-tests-cloud branch 2 times, most recently from ac84b48 to a1fc92d Compare March 22, 2022 22:17
Copy link
Contributor

@dnys1 dnys1 left a comment

Choose a reason for hiding this comment

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

Approved, contingent on changes mentioned

@HuiSF HuiSF force-pushed the chore/ds-integration-tests-cloud branch 4 times, most recently from 6e4aa74 to 2c62691 Compare March 25, 2022 18:03
@HuiSF HuiSF force-pushed the chore/ds-integration-tests-cloud branch 2 times, most recently from 46ed603 to c5d7078 Compare March 25, 2022 18:52
- Add reusable util for testing cloud sync opeartion
- Make integration tests configurable with
  - `--device-id` or `-d` to specify running device ID
  - `--enable-cloud-sync` or `-ec` to enable cloud sync in tests
@HuiSF HuiSF force-pushed the chore/ds-integration-tests-cloud branch from c5d7078 to 32c97cd Compare March 25, 2022 20:11
@HuiSF HuiSF merged commit 65f5b0f into aws-amplify:main Mar 25, 2022
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.

4 participants