search

awslabs / aws-mobile-appsync-sdk-android

Android SDK for AWS AppSync.
https://docs.amplify.aws/sdk/api/graphql/q/platform/android/
Apache License 2.0
105 stars 58 forks source link

Rewrite all instrumentation tests for legibility and correctness #256

Closed jamesonwilliams closed 4 years ago

jamesonwilliams commented 4 years ago

The instrumentation tests suffer from numerous factoring issues. Among the largest issues:

  1. AppSync callbacks occur on a different thread, but jUnit assertions are made inside of them. jUnit cannot catch the assertions, since they aren't occurring on the same thread as the test runner. To resolve this, several utilities are added that facilitate turning asynchronous calls into a synchronous ones (LatchedGraphQLCallback, LatchedSubscriptionCallback, Await, etc.)

  2. A call to AppSync requires instantiation of several different objects. In the tests, each component is cached into a local variable, and then all of them are composed together for a final call to AppSync. This makes it difficult to see what is actually being passed to AppSync. Instead, each object is inlined into a single big call to AppSync. This results in some dense statements, however it is easy to see which things go together, now.

  3. Lots of minor, Android Lint / code inspection issues that Android Studio will flag. Some of these include use of raw types or unchecked type conversions, object visibility escalation, dead-store methods and fields.

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

raphkim commented 4 years ago

Whoa this cleanup is great. I'm not familiar enough with AppSync to know if these tests have sufficient coverage especially since some of the existing ones are deleted. Can we get a confirmation?

jamesonwilliams commented 4 years ago

Whoa this cleanup is great. I'm not familiar enough with AppSync to know if these tests have sufficient coverage especially since some of the existing ones are deleted. Can we get a confirmation?

To be sure, I didn't delete any tests. This is 100% just re-arranging them. Actually, the tests had so many issues that I'm not sure they were really validating things they looked like they were.

So far I have verified that we have tests over different auth modes with queries and subscriptions. The test coverage could be worse.

And so much of the rest of the codebase is just actually Apollo, haha.

jamesonwilliams commented 4 years ago

(Double checked with @rjuliano before merging):

[June 8, 2020, 8:39 PM] Williams, Jameson: Should I merge the AppSync test changes I did? https://github.com/awslabs/aws-mobile-appsync-sdk-android/pull/256
[June 8, 2020, 8:39 PM] Juliano, Rafael: I would say so
[June 8, 2020, 8:39 PM] Williams, Jameson: Okay, cool