-
Notifications
You must be signed in to change notification settings - Fork 273
ci: add CircleCI config for running unit tests #9
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
Conversation
iartemiev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍🏻
Left a few comments/suggestions for your consideration. These don't necessarily have to be done as part of this PR.
.circleci/config.yml
Outdated
| git clone --branch ${FLUTTER_VERSION} https://github.com/flutter/flutter.git ${FLUTTER_HOME} | ||
| (yes || true) | flutter doctor --android-licenses && flutter doctor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be worth caching this with FLUTTER_VERSION as the cache key and then restoring from cache in each job, instead of having to clone it each time.
For reference: https://circleci.com/docs/2.0/caching/#full-example-of-saving-and-restoring-cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was considering use branch names such as "stable" and "beta" for the flutter_version parameter. I guess we can use git describe --tags to find the current version number for stable when saving the cache, but when restoring, is there a way to determine which version stable is on without cloning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. I had assumed that that's the Flutter release version, e.g., 1.12.13 based on the variable name. I think flutter_branch or flutter_tag might be a more descriptive variable name.
You could technically use the hash for the HEAD of whichever branch you want to clone, but then you'd need to store it somewhere, like in a file in a temp dir:
steps:
- run: echo | git ls-remote https://github.com/flutter/flutter.git refs/heads/master | \cut -f 1 > /tmp/flutter-hash
- restore_cache:
keys:
- v1-flutter-repo-${FLUTTER_VERSION}-{{ checksum "/tmp/flutter-hash" }}
- v1-flutter-repo-${FLUTTER_VERSION}
- v1-flutter-repoBut honestly, this is probably overkill / a premature optimization. I tested cloning the Flutter repo in CCI and it only takes ~7 seconds, whereas restoring from cache takes about 3s. So not worth the effort.
| executor: build-executor | ||
| steps: | ||
| - install_flutter | ||
| - checkout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment as with install_flutter.
I would add a job that checks out the code and caches it (using the commit hash as the key) or persists to workspace and then downstream jobs would restore cache or restore workspace, instead of checking out the repo from github in each job.
.circleci/config.yml
Outdated
| - checkout | ||
| - run: | ||
| name: Run Flutter Unit Tests | ||
| command: . .circleci/test_all_plugins.sh flutter-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be easier/more maintainable to just have the unit tests output JUnit xml reports that could then be consumed by https://circleci.com/docs/2.0/collect-test-data/ and displayed within the artifacts tab of the job, as opposed to using a custom bash script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main purpose of the bash script is to traverse the package directories. I will make the script generate JUnit XML reports when running tests in each package directory (and collect these XML reports at the end), but I can't think of a way to get rid of the bash script completely for now.
The Flutter team also uses a script to run the tests in different directories: https://github.com/flutter/plugin_tools/blob/master/lib/src/test_command.dart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Does the script run the tests sequentially or in parallel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's currently sequential. I don't think it would be too hard to make it parallel. Is it ok to do that in a future PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally!
|
After the Flutter 1.20 upgrade, the unit tests for iOS seem to be breaking (probably related to this issue: flutter/flutter#63018). I have been trying to figure out a fix, and I'll update this PR when I find out a fix before merging this. |
|
Still haven't found a solution for iOS unit tests on 1.20. Specifically, the 1.20 update is causing I guess until the problem is fixed, we will have to either go with only Dart and Kotlin unit tests for now, or stay at 1.17.5 where everything was working and wait for a fix... I understand that neither are good options. What are your opinions on this? |
|
@KaiChengYan I can't really comment from a platform perspective, as I'm fairly unfamiliar with Flutter. Can we just use the last known working version in the project (1.19 or whatever it was before) until this is fixed? @Amplifiyer @haverchuck - what do you guys think? |
|
We discussed during standup this morning, and the decision is that we delay iOS unit tests for now until we find a fix. The last stable version is 1.17.5, but we still want to stay on the latest version for the latest features and to ensure that our code compiles on latest stable. We will add e2e tests in the near future to cover iOS native code for now. iOS unit tests will be added once we figure out a solution. I will add a commit to remove iOS unit tests from the workflow. |
|
@KaiChengYan sounds good to me! |
| continue | ||
| fi | ||
| cp ~/amplify-flutter/.circleci/dummy_amplifyconfiguraton.dart example/lib/amplifyconfiguration.dart | ||
| cp ../../.circleci/dummy_amplifyconfiguraton.dart example/lib/amplifyconfiguration.dart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm storing a dummy amplifyconfiguration.dart file and copying it over to example/lib for each plugin. This is because the example/lib/main.dart imports the file, and won't build without it. The dummy file is only meant to make the build successful, and doesn't serve functional purposes.
@iartemiev Was this something that the native and JS libraries had to deal with? This seems to be new, b/c native and JS seem to use JSON for their amplify configs.
@haverchuck Do you think this would work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't say that we've had to do anything similar on native or JS. We just store the aws-exports files along with the sample apps, so when the integration tests run they simply consume those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iartemiev Thanks for the info. In our case the amplifyconfiguration.dart file is required even for unit tests, because the Dart code requires that file in compile-time.
Just discussed with @Ashish-Nanda about this. @Ashish-Nanda do you think the current approach (copying over a minimal config file) makes sense?
This reverts commit 59681ba.
Update Log Metric TEMP Ensure PR Trigger try fix fix formatting issues fix error 3 fix ending ) Pass github token fix syntax error remove smart quote BS Ensure Run temp update 2 temp Hopefully this fixes it Let’s try again Fix Attempt #4 fix failing step pipe fix improper job status fix dart script namings #7 fix freaking typo #8 fix mappings again #9 Print for Debug #10 Remove Quotes? aws-amplify#11 fixi failing trigger debug - print dart outputs fix syntax error try to get output force github actions to run fix syntax stop integ tests trynig to get output
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.