Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Conversation

@mvanbeusekom
Copy link
Contributor

@mvanbeusekom mvanbeusekom commented Mar 23, 2021

The current implementation of the JavaTestCommand only looks for a test folder under the ./example/android/app/src folder for a plugin. There are plugins that have their Java tests defined as part of the plugin implementation code (on path ./android/src/test) and don't have a test folder as part of the ./example/android/app/src path. This means these tests are currently not executed by the plugin CI. Some examples would be:

  • camera plugin
  • flutter_plugin_android_lifecycle
  • path_provider
  • url_launcher

We discovered this issue when we where trying to move the Java tests of the "in_app_purchase" plugin from the ./example/android/app/src/test folder to the ./android/src/test folder and noticed that the CI suddenly doesn't execute Java tests anymore (see PR #3732).

Note that I have fixed some Java unit tests of the camera plugin which have been failing unnoticed.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
    • The flutter_plugin_tools are not published anymore and doesn't include a version number.
  • I updated CHANGELOG.md to add a description of the change.
    • The flutter_plugin_tools are not published anymore and doesn't keep track of changes in a CHANGELOG.md
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Could we add test for the tools change. Also, are the camera tests change related to this PR?

@mvanbeusekom
Copy link
Contributor Author

change. Also, are the camera tests change related to this PR?

Yes I was already looking into if it is possible to test the change in the JavaTestCommand. I will give it another look tomorrow.

About the changes to the camera plugin, they fix the tests that are currently failing. This PR makes those failures visible and therefore will block the tree.

@mvanbeusekom
Copy link
Contributor Author

@cyanglaz I added tests to make sure the gradlew process is triggered when tests are located in either one of the expected locations:

  • As part of the example/android/app/src/test folder;
  • As part of the android/src/test folder.

ProcessCall(
p.join(plugin.path, 'example/android/gradlew'),
<String>['testDebugUnitTest', '--info'],
p.join(plugin.path, 'example/android'),
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be android/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this would be the working directory and it needs to point to the example/android location. From there it will look up the tests in android (through the compiled version of the example app).

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, my mistake.

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix!

@cyanglaz cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Mar 24, 2021
@fluttergithubbot fluttergithubbot merged commit 2904cbb into flutter:master Mar 24, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2021
@mvanbeusekom mvanbeusekom deleted the ci_run_java_tests branch September 21, 2021 09:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes p: camera platform-android waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants