Unit tests for AndroidProject and Builders #458

Menardi · 2018-06-27T08:10:20Z

Platforms affected

Android unit tests

What does this PR do?

I have written some more unit tests to continue increasing the coverage. This brings coverage to 54.94%. The coverage didn't increase as much as expected, as one of the files I have now written tests for (GenericBuilder.js) was not included at all in the previous coverage reports.

Unlike my previous tests, none of the files I have covered this time have reached 100%. This is because they are processing configuration files which I do not have good example of for testing. The main file I need a good example of is project.properties, with various entries. If there is one I could use, I am happy to complete the test coverage for these files.

Another thing to note is that GradleBuilder.js and StudioBuilder.js are very similar files, so the tests are pretty much copy-pasted. GenericBuilder.js appears to exist to pull out some functionality shared between the two, but it could definitely contain more. In a future PR, once I have complete test coverage of these files, I would like to break out the common code to make it more maintainable.

What testing has been done on this change?

Locally run npm run unit-tests

Checklist

Reported an issue in the JIRA database
Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
Added automated test coverage as appropriate for this change.

raphinesse · 2018-06-27T08:30:30Z

Thanks for the tests! I'll review them as soon as I find time.

Please take a look at #437. These two Builders also bothered me to the point that I wanted to refactor them but thankfully we might be able to get rid of one. Maybe @jcesarmobile can provide you with more details in case you want to tackle this.

raphinesse · 2018-06-27T08:32:44Z

Also note the test failures on windows due to unexpected path separators.

Menardi · 2018-06-27T08:56:05Z

Whoops, those failing Windows tests should be fixed now. I was wondering about the different builders and how they are used, thanks for linking that issue. I'll dig deeper and see what I can do.

codecov-io · 2018-06-27T09:14:35Z

Codecov Report

Merging #458 into master will increase coverage by 2.71%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
+ Coverage   52.03%   54.75%   +2.71%     
==========================================
  Files          17       18       +1     
  Lines        1695     1852     +157     
  Branches      312      333      +21     
==========================================
+ Hits          882     1014     +132     
- Misses        813      838      +25

Impacted Files	Coverage Δ
...in/templates/cordova/lib/builders/StudioBuilder.js	`45.22% <0%> (ø)`
bin/templates/cordova/lib/AndroidProject.js	`43.47% <0%> (+0.86%)`	⬆️
...in/templates/cordova/lib/builders/GradleBuilder.js	`45.91% <0%> (+25.15%)`	⬆️
...n/templates/cordova/lib/builders/GenericBuilder.js	`51.72% <0%> (+25.86%)`	⬆️
bin/templates/cordova/lib/builders/builders.js	`100% <0%> (+62.5%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46a036e...d76aff8. Read the comment docs.

jcesarmobile · 2018-06-27T09:53:36Z

I'm not really sure what the GradleBuilder is for, it was probably used before we had the StudioBuilder to build Eclipse Gradle projects, as Eclipse projects could be built with Ant or Gradle.

According to a Joe's comment on this PR, it's not used now that we only have Android Studio projects #437, so should be safe to delete.

All Android Studio projects are Gradle projects, that's probably why the GradleBuilder and StudioBuilder are similar.

raphinesse · 2018-06-27T10:10:33Z

All Android Studio projects are Gradle projects

@jcesarmobile Because of this fact I thought that we should name the remaining builder GradleBuilder. Especially if it might also be used to build non-Studio projects. Does that make sense?

raphinesse

First commit LGTM. Great tests as usual ❤️

raphinesse · 2018-06-27T10:24:19Z

spec/unit/AndroidProject.spec.js

+    });
+
+    describe('constructor', () => {
+        it('should set the project directory and www folder', () => {


This tests only the first part, right?

Ah yes, you're right. It originally tested project.www but I ended up breaking those into separate tests and forgot to rename this one.

raphinesse · 2018-06-27T10:28:49Z

spec/unit/AndroidProject.spec.js

+            expect(newProject).toEqual(jasmine.any(AndroidProject));
+        });
+
+        it('should return the existing project', () => {


Maybe "should cache created projects"?

raphinesse · 2018-06-27T10:32:42Z

spec/unit/AndroidProject.spec.js

+
+            const cache = AndroidProject.__get__('projectFileCache');
+            expect(Object.keys(cache).length).toBe(2);
+            expect(cache[projectToRemove]).toBe(undefined);


I find in to be vastly underused in the JS community 😉

Yeah, it wouldn't even cross my mind to use it!

raphinesse · 2018-06-27T10:44:38Z

spec/unit/AndroidProject.spec.js

@@ -113,15 +113,15 @@ describe('AndroidProject', () => {

            androidProject.getPackageName();

-            expect(AndroidManifestSpy).toHaveBeenCalledWith(`${PROJECT_DIR}/AndroidManifest.xml`);
+            expect(AndroidManifestSpy).toHaveBeenCalledWith(path.join(PROJECT_DIR, 'AndroidManifest.xml'));


That should go into the first commit

jcesarmobile · 2018-06-27T18:24:18Z

All Cordova projects are Android Studio projects. That just means that you can open them in Android Studio without problems and they will work if you run from there.

We are not going to build projects that are not compatible with Android Studio.

But as Android Studio projects are also Gradle projects, you don't need to open Android Studio to build them.

About the naming, I don't care much if we keep StudioBuilder or we keep the GradleBuilder and move Android Studio stuff there.

janpio · 2018-06-27T18:34:45Z

So one could rename the AndroidBuilder.js to e.g. ProjectBuilder.js, integrate GenericBuilder.js and clean the code to match this?

raphinesse · 2018-06-27T19:22:56Z

@janpio I like ProjectBuilder. Future proof naming for whatever Google throws at us 😄

Menardi · 2018-06-27T23:58:29Z

Thanks for all your feedback. It sounds like all the three builders could be made in to one. At that point do we still need builder.js, which is the intermediary that returns the requested builder? I'm not sure if its use is only within cordova-android, or if builders.getBuilder is a more general Cordova spec.

If it's not needed, then all of these files could be brought into a single file (ProjectBuilder.js), which would certainly make it a lot easier to understand.

As for the testing, I mentioned that I would like to fully cover the builders, but I don't have a good project.properties file to write the tests around. Do any of you have an example file with many entries, which I could use?

Menardi · 2018-06-28T23:49:37Z

I am working on refactoring the builders now. Since most of these tests will be outdated once that is done, I am going to close this PR. Later I will create a new PR with the builders refactor (and the appropriate unit tests). As for the tests unrelated to the builders in this PR, I will also submit them separately.

### Platforms affected Android ### What does this PR do? This is the last unit test PR for today, I promise! The `AndroidProject` tests were originally in #458. This PR also contains increased test coverage for `android_sdk.js`. I have also refactored that to remove `Q`, as with the other PRs I submitted today. ### What testing has been done on this change? Run unit tests ### Checklist - [x] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database - [x] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected. - [x] Added automated test coverage as appropriate for this change.

Menardi force-pushed the android_tests branch 2 times, most recently from 65cbe69 to d76aff8 Compare June 27, 2018 08:55

raphinesse reviewed Jun 27, 2018

View reviewed changes

Menardi force-pushed the android_tests branch from d76aff8 to 1cbabcb Compare June 27, 2018 23:53

Menardi added 2 commits June 28, 2018 16:37

Add some unit tests for AndroidProject.js

26775b7

Add builder tests

119382a

Menardi force-pushed the android_tests branch from 1cbabcb to 119382a Compare June 28, 2018 07:37

Menardi closed this Jun 28, 2018

This was referenced Jun 29, 2018

Remove old builders code #461

Merged

Unit tests for android_sdk and AndroidProject #464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests for AndroidProject and Builders #458

Unit tests for AndroidProject and Builders #458

Menardi commented Jun 27, 2018

raphinesse commented Jun 27, 2018

raphinesse commented Jun 27, 2018

Menardi commented Jun 27, 2018 •

edited

Loading

codecov-io commented Jun 27, 2018 •

edited

Loading

jcesarmobile commented Jun 27, 2018

raphinesse commented Jun 27, 2018

raphinesse left a comment

raphinesse Jun 27, 2018

Menardi Jun 27, 2018

raphinesse Jun 27, 2018

raphinesse Jun 27, 2018

Menardi Jun 27, 2018

raphinesse Jun 27, 2018 •

edited

Loading

jcesarmobile commented Jun 27, 2018

janpio commented Jun 27, 2018

raphinesse commented Jun 27, 2018

Menardi commented Jun 27, 2018

Menardi commented Jun 28, 2018

Unit tests for AndroidProject and Builders #458

Unit tests for AndroidProject and Builders #458

Conversation

Menardi commented Jun 27, 2018

Platforms affected

What does this PR do?

What testing has been done on this change?

Checklist

raphinesse commented Jun 27, 2018

raphinesse commented Jun 27, 2018

Menardi commented Jun 27, 2018 • edited Loading

codecov-io commented Jun 27, 2018 • edited Loading

Codecov Report

jcesarmobile commented Jun 27, 2018

raphinesse commented Jun 27, 2018

raphinesse left a comment

Choose a reason for hiding this comment

raphinesse Jun 27, 2018

Choose a reason for hiding this comment

Menardi Jun 27, 2018

Choose a reason for hiding this comment

raphinesse Jun 27, 2018

Choose a reason for hiding this comment

raphinesse Jun 27, 2018

Choose a reason for hiding this comment

Menardi Jun 27, 2018

Choose a reason for hiding this comment

raphinesse Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

jcesarmobile commented Jun 27, 2018

janpio commented Jun 27, 2018

raphinesse commented Jun 27, 2018

Menardi commented Jun 27, 2018

Menardi commented Jun 28, 2018

Menardi commented Jun 27, 2018 •

edited

Loading

codecov-io commented Jun 27, 2018 •

edited

Loading

raphinesse Jun 27, 2018 •

edited

Loading