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

Android Native Reporter #8

Merged
merged 16 commits into from
Jun 4, 2019
Merged

Android Native Reporter #8

merged 16 commits into from
Jun 4, 2019

Conversation

AbigailMcP
Copy link
Contributor

@AbigailMcP AbigailMcP commented May 28, 2019

Includes:

  • Android native bridge module that can be used to wait for Cavy tests to complete with a report.
  • Example of an Android test using this module.

To do:

  • Update example to make an assertion based on the contents of the report i.e. check for errorCount
  • Update README with an example of how to use module
  • Code comments

@AbigailMcP AbigailMcP requested a review from jalada May 28, 2019 15:06
README.md Outdated
@Test
public void testBridge() throws Exception {
// Wait 5 seconds to receive a test report from Cavy.
RNCavyNativeReporterModule.waitForReport(5);
Copy link
Contributor

Choose a reason for hiding this comment

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

that's way too low for a default, given that for a sizable app, first 10-30 seconds might be spent by bundler just bundling the app… And CIs are slow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

[expectation fulfill];
}];


// Wait for expectation to fulfill.
[self waitForExpectations:@[expectation] timeout:100];
Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion, to make this simpler for non-platform experts is to also supply this code from the Cavy bundle. Then we could simplify it to:

- (void)testBridge {
   [CavyXCTest testBridgeWithTimeout: 100];
}

Copy link
Contributor Author

@AbigailMcP AbigailMcP May 30, 2019

Choose a reason for hiding this comment

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

You're right, this would look really neat 👌 I'm conscious that this would couple the native reporter to XCTest only..

I agree that the API could be improved though. I had a think about implementing this in a non XCTest specific way using DispatchGroups, so you could have something like:

- (void)testBridge {
  [CavyNativeReporter waitForReport:5];
 
  NSDictionary *report = CavyNativeReporter.cavyReport;
  long errorCount = [report[@"errorCount"] integerValue];
  
  XCTAssertEqual(errorCount, 0);
}

but it's a definite work in progress!

I'm going to track the idea as an issue and continue to work on it :)

@radex
Copy link
Contributor

radex commented May 29, 2019

One more general suggestion (sorry to barge in on an unrelated PR) is that it would be amazing if Cavy / CavyNativeReported had a mechanism to detect a bundling failure.

What I mean is that we saw many times test fail with the assertion that it timed out (after 10 minuets in our case)… but in reality it didn't time out but tests never began to run, because there was a bundling failure and the JS code never launched. Perhaps it would be possible to observe in AppDelegate/MainActivity whether or not loading JS into RCTRootView was successful or not and then send that message to the test part? Or if not successful throw a native error so that it's immediately obvious in logs that it's bundling that failed, and so that we don't have to wait for the timeout (for large app/library with a lot of tests, a large timeout like 10 minutes might be necessary - and then a failure makes you wait a loooong time for CI).

At the very least, I would suggest adding a message to the timeout to let the user know that the timeout might mean that A) Your tests are running for a very long time and you might need to increase the time out, or B) Most likely you had a bundler failure or an early-stage app failure and Cavy never set up successfully

@AbigailMcP
Copy link
Contributor Author

Hey @radex - thanks so much for your input, super useful.

Great point re bundling timeout, I think I'll go down the messaging route first and add the trickier bundle failure error handling as a separate issue to tackle later 👌

@jalada - would be great to get your thoughts on this too!

This was referenced May 30, 2019
@jalada
Copy link
Contributor

jalada commented May 31, 2019

I got this up and running pretty quickly!

  • Give a tip on opening Android Studio (open the android/ folder not the project root)
  • Give an example of where the bridge test should be. It's kinda tricky to make a src/androidTest folder, maybe tell people about swapping to the Project view in Android Studio, I couldn't find a more accepted way of doing this.
  • List the dependencies you need to add to android/app/build.gradle.
  • Mention that you need to add testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner" to the defaultConfig in android/app/build.gradle.
  • Comment on the import com.sampleapp.MainActivity; line that this should be the identifier for your own app's main activity.
  • assertEquals is the wrong way around.

I'm getting a warning from Android Studio, which we might want to ignore, not sure?

WARNING: Configuration 'compile' is obsolete and has been replaced with 'implementation' and 'api'.
It will be removed at the end of 2018. For more information see: http://d.android.com/r/tools/update-dependency-configurations.html
Affected Modules: cavy-native-reporter


WARNING: The specified Android SDK Build Tools version (28.0.0) is ignored, as it is below the minimum supported version (28.0.3) for Android Gradle Plugin 3.3.1.
Android SDK Build Tools 28.0.3 will be used.
To suppress this warning, remove "buildToolsVersion '28.0.0'" from your build.gradle file, as each version of the Android Gradle Plugin now has a default version of the build tools.
Remove Build Tools version and sync project
Affected Modules: cavy-native-reporter

@AbigailMcP
Copy link
Contributor Author

@jalada - updated for all of the above.

I also fixed both the deprecation warnings and added a 'useful links' section for both Android and iOS.

@jalada
Copy link
Contributor

jalada commented Jun 4, 2019

:shipit:

@AbigailMcP AbigailMcP merged commit 4e71e50 into master Jun 4, 2019
@AbigailMcP AbigailMcP deleted the android branch June 4, 2019 09:02
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.

3 participants