Remove old builders code #461

Menardi · 2018-06-29T00:38:26Z

Platforms affected

Android

What does this PR do?

This refactors the builders to remove legacy code, as per discussions in #458 and #437.

GradleBuilder has been removed as it was not being used. StudioBuilder has been renamed to ProjectBuilder as several people suggested in #458. GenericBuilder has also been merged into this file. I have kept builder.js, which is used to access the builder from other files. Since there is now only one builder, we probably don't need to keep this, but it minimises external changes. The tests which were originally part of #458 are now in here in ProjectBuilder.spec.js.

I have removed AndroidStudio.js since it only had one function, which always returned true.

In the last commit, I refactored ProjectBuilder.js to use class instead of prototype since the minimum Node version is now 6. Functionally it makes no difference, but I find it easier to read. Maybe not everyone will agree, which is why I put it in a separate commit which can be removed if anyone strongly objects.

Due to the change to class, there are a lot of whitespace changes. For reviewing, I recommend adding ?w=1 to the end of the GitHub URL to ignore whitespace changes. It should make things a bit easier. Also, it's probably best to go commit-by-commit, as reviewing both commits together for some reason shows ProjectBuilder.js as a new file rather than a renamed StudioBuilder.js.

What testing has been done on this change?

I think the testing below is sufficient, but please let me know if there is more I should do.

Created a project using ./bin/create
Built this project using ./cordova/build and successfully ran on an emulator
Ran JS and Java 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.

codecov-io · 2018-06-29T00:49:55Z

Codecov Report

Merging #461 into master will increase coverage by 2.34%.
The diff coverage is 40.7%.

@@            Coverage Diff            @@
##           master    #461      +/-   ##
=========================================
+ Coverage   60.56%   62.9%   +2.34%     
=========================================
  Files          17      15       -2     
  Lines        1694    1650      -44     
  Branches      312     308       -4     
=========================================
+ Hits         1026    1038      +12     
+ Misses        668     612      -56

Impacted Files	Coverage Δ
bin/templates/cordova/lib/builders/builders.js	`100% <ø> (+62.5%)`	⬆️
bin/templates/cordova/lib/AndroidProject.js	`40% <100%> (-2.61%)`	⬇️
bin/templates/cordova/Api.js	`41.58% <100%> (-7.99%)`	⬇️
...n/templates/cordova/lib/builders/ProjectBuilder.js	`39.79% <39.79%> (ø)`

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 b6e4598...4b0725d. Read the comment docs.

janpio · 2018-06-29T10:55:37Z

bin/templates/cordova/Api.js

@@ -56,41 +55,27 @@ function setupEvents (externalEventEmitter) {
 function Api (platform, platformRootDir, events) {
    this.platform = PLATFORM;
    this.root = path.resolve(__dirname, '..');
-    this.builder = 'gradle';
+    this.builder = 'studio';


if there is only one builder, then this could probably be removed?

I wasn't sure if other things relied on Api.builder so I left it alone. The refactor could do a lot more, but I was reluctant to change too much given that I am still relatively unfamiliar with the codebase.

dpogue

👍

raphinesse

Left a few comments. Thanks for your work!

raphinesse · 2018-07-08T16:29:00Z

bin/templates/cordova/lib/builders/ProjectBuilder.js

-var SIGNING_PROPERTIES = '-signing.properties';
-var TEMPLATE =
+/* eslint no-self-assign: 0 */
+/* eslint no-unused-vars: 0 */


I don't like introducing these comments, especially file global. Why do we need these?

I didn't actually introduce these, they are originally from GenericBuilder. They cover this line. It makes no sense to me, but I left it as is for now.

raphinesse · 2018-07-08T16:29:42Z

bin/templates/cordova/lib/builders/ProjectBuilder.js

+let spawn = require('cordova-common').superspawn.spawn;
+let events = require('cordova-common').events;
+let CordovaError = require('cordova-common').CordovaError;
+let check_reqs = require('../check_reqs');


Why can't we use const here too? Or at least leave them using var for now.

Using const means that we can't spy on them unfortunately. I can revert them to var if you think that would be better.

Using const means that we can't spy on them unfortunately.

I don't get why that would be the case : thinking:

I can revert them to var if you think that would be better.

I'd prefer that, since it reduces the amount of unrelated changes.

raphinesse · 2018-07-08T16:31:41Z

bin/templates/cordova/lib/builders/ProjectBuilder.js

+    this.root = projectRoot || path.resolve(__dirname, '../../..');
+    this.binDirs = {
+        studio: path.join(this.root, 'app', 'build', 'outputs', 'apk'),
+        gradle: path.join(this.root, 'app', 'build', 'outputs', 'apk')


Why do we need both of these?

I was attempting to keep potential breaking changes to a minimum. It is unlikely that binDirs.gradle is being used, but I was unsure about removing it.

raphinesse · 2018-07-08T16:39:45Z

spec/unit/builders/ProjectBuilder.spec.js

+            expect(builder.root).toBe(rootDir);
+        });
+
+        it('should set the project directory to three folders back', () => {


"… three folders up"?

raphinesse · 2018-07-08T16:44:52Z

spec/unit/builders/ProjectBuilder.spec.js

+            spyOn(builder, 'getArgs');
+        });
+
+        it('should set to build type to debug', () => {


One "to" too many

raphinesse · 2018-07-08T16:48:32Z

spec/unit/builders/ProjectBuilder.spec.js

+    let spawnSpy;
+
+    beforeEach(() => {
+        spawnSpy = jasmine.createSpy('spawn').and.returnValue(Q.defer().promise);


Does the calling code expect spawn to return a Q instance?

Yes, it uses .progress, which is not part of the Promise spec. It is only used as a workaround for CB-9971, so it should be possible to remove it at some point.

raphinesse · 2018-07-08T16:59:37Z

spec/unit/builders/builders.spec.js

+        it('should return an instance of ProjectBuilder', () => {
+            const newBuilder = builder.getBuilder('studio');
+            expect(newBuilder).toEqual(jasmine.any(ProjectBuilder));
+        });


Duplicated test

The first tests when gradle is requested, and the second when studio is requested. I will rename the tests to make the difference more obvious.

raphinesse · 2018-07-08T17:00:38Z

spec/unit/builders/builders.spec.js

+const ProjectBuilder = require('../../../bin/templates/cordova/lib/builders/ProjectBuilder');
+
+describe('builders', () => {
+    let builder;


Wouldn't builders be a more appropriate name?

raphinesse · 2018-07-10T07:27:07Z

Thanks for addressing my comments @Menardi. I think we can merge this. Agreed, @dpogue?

It would be great if you could somehow document all the strange stuff you came about that should be addressed but was outside the scope of this PR or potentially unsafe. Either as an issue or by creating another PR that errs on the unsafe side.

Thanks again for the good work!

Menardi · 2018-07-11T00:11:10Z

Once this is merged, I am happy to take another look through the code to see how much more refactoring I can do relating to the builders. I think there is a lot of improvement to be had.

- General Code Refactor - Removed builder type argument from getBuilder API - Removed any reference of conditional statements around builder type - Remove plugin handler install and uninstall option flag android_studio - Remove --gradle flag references - Fixed plugin handler install and uninstall pathing issues - Removed the second build process. The build before launching emulator has been removed. Only run the build once and after the emulator start process is triggered. - Added parseBuildOptions export so run can get build related options. - Use the nobuild flag option to control the run build. - Updated test spec to reflect the changes.

- General Code Refactor - Removed builder type argument from getBuilder API - Removed any reference of conditional statements around builder type - Remove plugin handler install and uninstall option flag android_studio - Remove --gradle flag references - Fixed plugin handler install and uninstall pathing issues - Added parseBuildOptions export so run can get build related options. - Use the nobuild flag option to control the run build. - Updated test spec to reflect the changes.

Menardi changed the title ~~Refactor builders (with unit tests)~~ Remove old builders code Jun 29, 2018

janpio reviewed Jun 29, 2018

View reviewed changes

apache deleted a comment from asilkarbeyaz Jun 29, 2018

dpogue approved these changes Jul 3, 2018

View reviewed changes

raphinesse reviewed Jul 8, 2018

View reviewed changes

Menardi force-pushed the refactor_builders_with_tests branch from 02a9f43 to 1e012ec Compare July 10, 2018 00:05

Menardi added 3 commits July 10, 2018 11:53

Remove unused builders and add unit tests

8ee3a73

Refactor ProjectBuilder to use class instead of prototype

350d35f

Remove no longer needed AndroidStudio.js

4b0725d

Menardi force-pushed the refactor_builders_with_tests branch from 1e012ec to 4b0725d Compare July 10, 2018 02:54

raphinesse merged commit ebbd91f into apache:master Jul 21, 2018

Menardi deleted the refactor_builders_with_tests branch July 23, 2018 11:36

Menardi mentioned this pull request Sep 1, 2018

Run command fails if build is skipped #488

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove old builders code #461

Remove old builders code #461

Menardi commented Jun 29, 2018 •

edited

Loading

codecov-io commented Jun 29, 2018 •

edited

Loading

janpio Jun 29, 2018

Menardi Jul 1, 2018

dpogue left a comment

raphinesse left a comment

raphinesse Jul 8, 2018

Menardi Jul 9, 2018

raphinesse Jul 10, 2018

raphinesse Jul 8, 2018

Menardi Jul 9, 2018

raphinesse Jul 10, 2018

raphinesse Jul 8, 2018

Menardi Jul 9, 2018

raphinesse Jul 8, 2018

raphinesse Jul 8, 2018

raphinesse Jul 8, 2018

Menardi Jul 10, 2018

raphinesse Jul 8, 2018

Menardi Jul 10, 2018

raphinesse Jul 8, 2018

raphinesse commented Jul 10, 2018

Menardi commented Jul 11, 2018

Remove old builders code #461

Remove old builders code #461

Conversation

Menardi commented Jun 29, 2018 • edited Loading

Platforms affected

What does this PR do?

What testing has been done on this change?

Checklist

codecov-io commented Jun 29, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpogue left a comment

Choose a reason for hiding this comment

raphinesse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphinesse commented Jul 10, 2018

Menardi commented Jul 11, 2018

Menardi commented Jun 29, 2018 •

edited

Loading

codecov-io commented Jun 29, 2018 •

edited

Loading