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

Test JSObject #2508

Merged
merged 13 commits into from
Jun 24, 2020
Merged

Test JSObject #2508

merged 13 commits into from
Jun 24, 2020

Conversation

imjacobclark
Copy link
Contributor

@imjacobclark imjacobclark commented Mar 2, 2020

First contribution here...

I plan on contributing more than these tests in this PR, but I chose JSObject as a first good place to start.

Feedback/thoughts welcome!

Thanks

Copy link
Contributor

@smpeters smpeters left a comment

Choose a reason for hiding this comment

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

Thanks for this! I've been a bit concerned with the lack of tests in the Capacitor code so it'd be great to have some.

@@ -54,6 +54,9 @@ dependencies {
implementation 'com.google.firebase:firebase-messaging:18.0.0'
implementation 'com.google.android.gms:play-services-location:17.0.0'
testImplementation 'junit:junit:4.12'
testImplementation 'org.json:json:20140107'


Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is needed. The only import the test code has that uses this is import org.json.JSONException; and that's included in Android API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to prevent java.lang.RuntimeException: Method getString in org.json.JSONObject not mocked. See http://g.co/androidstudio/not-mocked for details. errors as JSON is bundled with Android and thus hitting a stub, so a real JSON implementation is needed to drive a real valuable test.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that explanation is fine with me.

@imhoffd
Copy link
Contributor

imhoffd commented Jun 23, 2020

@imjacobclark Any comment on the above review? Also, this repo uses GitHub Actions, would you be able to update the PR?

@imjacobclark
Copy link
Contributor Author

@imjacobclark Any comment on the above review? Also, this repo uses GitHub Actions, would you be able to update the PR?

Hey @dwieeb @smpeters thanks for the review - it’s on my todo list to fix this up, I’ll get around to it tomorrow I think!

@imjacobclark
Copy link
Contributor Author

@dwieeb @smpeters resolved the static imports, updated GitHub actions, however left a comment on the outstanding requested change, as I believe that is still required!

Thanks

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@imhoffd
Copy link
Contributor

imhoffd commented Jun 24, 2020

LGTM! Thanks for getting Capacitor set up with testing in Java. 😅

@imjacobclark
Copy link
Contributor Author

LGTM! Thanks for getting Capacitor set up with testing in Java. 😅

@dwieeb no problem! What’s the process to get this merged in?

@imhoffd
Copy link
Contributor

imhoffd commented Jun 24, 2020

Couple things yet...

@smpeters Would you care to comment further or resolve the conversation in your review?
@carlpoole Care to take a look?

@imhoffd imhoffd requested a review from carlpoole June 24, 2020 18:53
@smpeters
Copy link
Contributor

Couple things yet...

@smpeters Would you care to comment further or resolve the conversation in your review?
@carlpoole Care to take a look?

LGTM

@imhoffd imhoffd merged commit 5a37496 into ionic-team:master Jun 24, 2020
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.

None yet

4 participants