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): run integ tests in CI #2182

Merged
merged 8 commits into from
Oct 3, 2022
Merged

chore(datastore): run integ tests in CI #2182

merged 8 commits into from
Oct 3, 2022

Conversation

ragingsquirrel3
Copy link
Contributor

@ragingsquirrel3 ragingsquirrel3 commented Sep 27, 2022

This PR enables subset of datastore integ tests in CI. In summary:

  • changes to GH action config to run datastore tests in CI
  • adds "small" parameter for integ tests although datastore is far only test to differ logic here. For datastore, it will only run the main_test.dart (lots of offline tests) and one basic test for cloud sync. This is necessary because running all the datastore tests same way as other tests will cause increase in flakiness/resource consumption that we don't want for current test execution environment. Eventually, might be good to have a "large" suite that runs tests like this once a week or something like that.
  • tweak a sorting test that was failing in CI as written because 2 AWSDates are the same and get into different orders.

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

@ragingsquirrel3 ragingsquirrel3 marked this pull request as ready for review September 29, 2022 18:53
@ragingsquirrel3 ragingsquirrel3 requested a review from a team as a code owner September 29, 2022 18:53
with:
# API levels 30+ too slow https://github.com/ReactiveCircus/android-emulator-runner/issues/222
api-level: 29
script: melos exec -c 1 --scope ${{ matrix.scope }} -- "deviceId=emulator-5554 retries=1 \$MELOS_ROOT_PATH/build-support/integ_test_android.sh"
script: melos exec -c 1 --scope ${{ matrix.scope }} -- "deviceId=emulator-5554 retries=1 small=true \$MELOS_ROOT_PATH/build-support/integ_test_android.sh"
Copy link
Member

Choose a reason for hiding this comment

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

Could we make use of tags instead of the small/large flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looked into this and had some issues, will ping you offline.

@@ -104,7 +104,7 @@ void main() {
DateTime(0000, 01, 01, 00, 00, 00),
DateTime(1970, 01, 01, 00, 00, 00),
DateTime(2020, 01, 01, 00, 00, 00),
DateTime(2020, 01, 01, 23, 59, 59),
DateTime(2020, 01, 02, 23, 59, 59),
Copy link
Member

Choose a reason for hiding this comment

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

Was the test failing without this change? Might be a bug?

Copy link
Contributor Author

@ragingsquirrel3 ragingsquirrel3 Sep 29, 2022

Choose a reason for hiding this comment

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

Yes was failing, although not bug IMO. Discussed offline w @HuiSF but was failing consistently in CI on android/ios but not locally. These 2 values get converted to the same date and their order would get switched. Not sure why their order gets switched as I understand should return in save in order when the date equal? though not clear if that's a requirement. However, this test should just test the date order not the order when date equals.

If expected behavior is indeed that should be in some order even when the same date, there should be a separate test for that IMO, and it's a bug.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks for the explanation. I thought this test was for AWSDateTime. I see it is for AWSDate. It is interesting that it was only failing in CI, but I agree there is no expectation for order in this case.

@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2022

Codecov Report

Merging #2182 (5e09e46) into main (4839c47) will increase coverage by 0.30%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2182      +/-   ##
==========================================
+ Coverage   45.94%   46.24%   +0.30%     
==========================================
  Files         359      363       +4     
  Lines       10871    10973     +102     
==========================================
+ Hits         4995     5075      +80     
- Misses       5876     5898      +22     
Flag Coverage Δ
android-unit-tests ∅ <ø> (∅)
flutter-unit-tests 35.89% <ø> (ø)
ios-unit-tests 89.28% <ø> (-0.55%) ⬇️

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

Impacted Files Coverage Δ
.../ios/unit_tests/amplify_flutter_exampleTests.swift 94.59% <0.00%> (ø)
...e/ios/unit_tests/MockAnalyticsCategoryPlugin.swift 15.38% <0.00%> (ø)
...y_flutter_ios/example/ios/Runner/AppDelegate.swift 0.00% <0.00%> (ø)
...ios/example/ios/unit_tests/AtomicResultTests.swift 87.75% <0.00%> (ø)

@@ -104,7 +104,7 @@ void main() {
DateTime(0000, 01, 01, 00, 00, 00),
DateTime(1970, 01, 01, 00, 00, 00),
DateTime(2020, 01, 01, 00, 00, 00),
DateTime(2020, 01, 01, 23, 59, 59),
DateTime(2020, 01, 02, 23, 59, 59),
Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks for the explanation. I thought this test was for AWSDateTime. I see it is for AWSDate. It is interesting that it was only failing in CI, but I agree there is no expectation for order in this case.

@ragingsquirrel3 ragingsquirrel3 merged commit 7307233 into aws-amplify:main Oct 3, 2022
@ragingsquirrel3 ragingsquirrel3 deleted the feat/datastore-integ-ci branch October 3, 2022 17:29
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