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

Conversation

@Zazo032
Copy link
Contributor

@Zazo032 Zazo032 commented Sep 30, 2019

Description

Trivial change to properly check if the type of an event is an error, so a description can be added.

Related Issues

Fixes flutter/flutter#41601

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
    • Trivial change
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

event.put("eventType", eventType.toString().toLowerCase());
// Only errors have description
if (eventType != EventType.ERROR) {
if (eventType == EventType.ERROR) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops. Thank you for fixing this.

@mklim
Copy link
Contributor

mklim commented Sep 30, 2019

Thank you for the fix!

We've recently re-emphasized that all PRs need tests, following the style guide.

This is difficult for plugins PRs since we don't have an integrated testing solution, but I don't think it's impossible to do here. Normally I would recommend writing a flutter driver test that tries to do something invalid and then make sure the error has a description, but tests for this plugin need to somehow automatically accept the camera permission to run and we don't have a good cross platform solution for that yet.

We do have the ability to write JUnit tests though, and I think that would also work. Would you be able to create a CameraTests.java class with a unit test making sure messages are sent with a description? I think the easiest way to do this would be to open example/app/android in Android studio, and then use it to automatically make and run a new unit test class for Camera.java.

If you're not able to carry this forward, please let us know and we'll be happy to shepherd it on from here. Thanks again for the contribution.

@Zazo032
Copy link
Contributor Author

Zazo032 commented Oct 1, 2019

I have been trying to generate a test class for Camera.java but Android Studio is ignorning me.

image

Pressing "OK" does nothing, tried checking/unchecking different methods, using Groovy/JUnit 4/JUnit 5 but Android Studio just ignores me, not sure if I'm missing something else.

Michael Klimushyn added 2 commits October 1, 2019 15:32
Break out the functionality related to sending Dart callbacks into its
own class, DartMessenger. This will let us test the former `sendEvent`
without needing to initialize the camera hardware.
@mklim
Copy link
Contributor

mklim commented Oct 1, 2019

Thanks for trying! I opened up Android Studio myself to add in a "quick test" skeleton for you to pick up and then realized that actually properly unit testing this fix is actually really difficult. The class wasn't set up to be testable. I didn't see a way to do it without refactoring the send() logic out into a new class. Even instantiating Camera depends on having hardware mocked out at a level that was really difficult to do. I ended up at a point where I was trying to make custom Robolectric shadows for the camera hardware and still couldn't cover everything.

So instead I ended up making two commits, first to refactor the class and then to actually test this method. What do you think about cherry picking them into this PR and then we can land it all together?

@Zazo032
Copy link
Contributor Author

Zazo032 commented Oct 2, 2019

Not sure if cherry picked or just merged (a bit new to this tools, excuse me if this isn't the correct process), but tests are passing correctly so this is probably ready to merge (waiting for review approvals)

Copy link
Contributor

@mklim mklim 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 again!

@mklim mklim merged commit c3f07e7 into flutter:master Oct 2, 2019
mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019
Trivial change to properly check if the type of an event is an error, so a description can be added.
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
Trivial change to properly check if the type of an event is an error, so a description can be added.
Akachu pushed a commit to Akachu/flutter_camera that referenced this pull request Apr 27, 2020
Trivial change to properly check if the type of an event is an error, so a description can be added.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Camera] Camera plugin Bug

3 participants