Support watch2 apps/extensions

[l3ender]

Resolves #55.

[l3ender]

From my testing, this is all that I needed to do in order to add watch2 apps and app extensions. Could a maintainer take a look to see if this could be merged? Let me know if there's anything else I can help with.

Thanks!

[brodycj]

I will probably need 2-3 weeks to look at this one. I think the test suite needs to be updated to reduce the chance of breaking this functionality in the future.

[l3ender]

Is there anything I can do to assist and help speed the process along?

[brodycj]

Requesting changes due to lack of test updates. We really need test updates to reduce the chance of accidentally breaking this update in the future.

[brodycj]

We would be happy to review this proposal once the update is covered by the test suite. Please do not hesitate to ask in case you have any questions.

You can always ping us at dev@cordova.apache.org or via the Slack channel. (I am not so active on Slack, through.) Some patience would be appreciated as I think all maintainers are overworked and not overpaid.

[l3ender]

Thanks for the detail. I found that the watch ability wasn't actually fully supported: when adding a watch app/extension, it wasn't actually creating/linking dependency and copying files. I've updated to support watch2 but since it currently isn't implemented for watch1 I want to leave that untouched (also, I don't have a sample project with a watch1 app/extension).

I'll dive into the test suite this coming week to add coverage for the items I've modified. In the meantime, if you have any comments on the changes please let me know. Thanks!

[l3ender]

Hi @brodybits - after letting this fall off my plate, I've added test coverage for watch apps/extensions. Can you review and let me know if there's anything else I can add?

[brodycj]

Considering my lack of familiarity with watchkit2, I did mutation testing using Stryker to help ensure we do not lose any code coverage (see PR #61). Since all lib changes are in lib/pbxProject.js, we do not have to check for loss of code coverage in any other modules. Here are the HTML reports with and without this proposal for the sake of comparison:

• https://chrisbrody.com/node-xcode-mutation/index.html - without this proposal
• https://chrisbrody.com/node-xcode-mutation-with-watchkit2-ability/index.html - with this proposal

Stryker discovered that the following mutations of the proposed changes would not be detected by the added tests:

Mutant 1:

diff --git a/lib/pbxProject.js b/lib/pbxProject.js
index 48b6acd..e8624df 100644
--- a/lib/pbxProject.js
+++ b/lib/pbxProject.js
@@ -1486,7 +1486,7 @@ pbxProject.prototype.addTarget = function(name, type, subfolder) {
             targetType,
             '"$(CONTENTS_FOLDER_PATH)/Watch"'
         );
-    } else if (targetType === 'watch2_extension') {
+    } else if (true) {
         // Create CopyFiles phase in watch target (if exists)
         var watch2Target = this.getTarget(producttypeForTargettype('watch2_app'));
         if (watch2Target) {

This mutant is directly related to missing test coverage of build with app_extension target, now tracked in #63.

Mutant 2:

diff --git a/lib/pbxProject.js b/lib/pbxProject.js
index 48b6acd..43de7af 100644
--- a/lib/pbxProject.js
+++ b/lib/pbxProject.js
@@ -1491,7 +1491,7 @@ pbxProject.prototype.addTarget = function(name, type, subfolder) {
         var watch2Target = this.getTarget(producttypeForTargettype('watch2_app'));
         if (watch2Target) {
             this.addBuildPhase(
-                [targetName + '.appex'],
+                [targetName + ''],
                 'PBXCopyFilesBuildPhase',
                 'Embed App Extensions',
                 watch2Target.uuid,

In addition, I noticed that the existing WatchKit code is not covered by the existing test cases. I tried adapting the new test cases for the original WatchKit support in brodycj#1 (on my own fork).

[l3ender]

Would you like me to take a look at #63 (coverage for app_extension) as it looks like you're already working on coverage for watch1 app/extension?

[brodycj]

Hi Bender, I would rather that you help with "watch 1" first, if it is ok with you. I copied your test into a fork PR for the sake of discussion only. I would also be grateful if you want to look at the app_extension coverage in #63. I will be taking some on-and-off break days over the next 1-2 weeks. I will continue to monitor every 1-2 days. Thanks again!

…

[l3ender]

Hi @brodybits - sorry for the delay on my part. I've taken a look at trying to add coverage for original WatchKit apps (AKA "watch1") but I have some doubt on being able to validate changes as compared to Xcode generated projects. I can't find a way to create an original WatchKit app/extension in Xcode (only WatchKit2), so I'm not able to confirm what is expected output in a pbxproj file.

I can add in coverage with how I believe it should work based on my [limited] knowledge of WatchKit2, but wanted to first see if you think that is worth the risk of adding coverage without certainty of desired outcome.

I'm happy to help either way--let me know. Thanks!

[l3ender]

I added better coverage for the mutants above--let me know if there's anything else I can take a look at. Thanks!

[brodycj]

Thanks. I checked that the mutants are now detected by the test suite.

I think that most but not all of the test cases you added would work on both the existing WatchKit functionality and WatchKit 2.

I would like to add test coverage of the existing WatchKit functionality, like I started in brodycj#1, to ensure it is not broken by these changes. I may need a few days to do this.

[brodycj]

I just updated https://chrisbrody.com/node-xcode-mutation-with-watchkit2-ability/index.html - unfortunately I do still see a few more mutants that we should try to kill. I would like to take care of testing the existing WatchKit functionality before dealing with the remaining WatchKit 2 mutants.

[brodycj]

I think we need to do the following before merging:

I raised PR #71 to test the existing WatckKit support, need some review feedback from @l3ender.

Once we merge PR #71, I think these changes need to be rebased and tests updated to test existing WatchKit vs WatchKit 2. Up to @l3ender whether or not to move the WatchKit 2 tests into separate modules.

Internal filetypeForProducttype function that is needed for WatchKit support does not have proper test coverage, see issue #70. Seems to be a surviving mutant.

And one other surviving mutant:

diff --git a/lib/pbxProject.js b/lib/pbxProject.js
index 92182f0..fa2e74a 100644
--- a/lib/pbxProject.js
+++ b/lib/pbxProject.js
@@ -1479,7 +1479,7 @@ pbxProject.prototype.addTarget = function(name, type, subfolder) {
     } else if (targetType === 'watch2_app') {
         // Create CopyFiles phase in first target
         this.addBuildPhase(
-            [targetName + '.app'],
+            [targetName + ''],
             'PBXCopyFilesBuildPhase',
             'Embed Watch Content',
             this.getFirstTarget().uuid,

Thanks!

[l3ender]

This should be good to go:

• brought in latest from master
• moved watch2 tests to separate modules
• ensured coverage added in master was also added for watch2
• covered mutant above (Support watch2 apps/extensions #56 (review)).

Attaching updated Stryker report: bind-mutation-test-report.js.txt

[brodycj]

Thanks, I updated the report at: https://chrisbrody.com/node-xcode-mutation-with-watchkit2-ability/index.html

I discovered one more surviving mutant in the proposed changes:

diff --git a/lib/pbxProject.js b/lib/pbxProject.js
index e065d82..d2ed07a 100644
--- a/lib/pbxProject.js
+++ b/lib/pbxProject.js
@@ -1510,8 +1510,6 @@ pbxProject.prototype.addTarget = function(name, type, subfolder) {
             this.addTargetDependency(watch2Target.uuid, [target.uuid]);
         }
     } else {
-        // Target: Add dependency for this target to first (main) target
-        this.addTargetDependency(this.getFirstTarget().uuid, [target.uuid]);
     }
 
 

I think this is the result of the mutant that I reported in #73. I think this mutant should be tested in a separate PR, to resolve #73, before we merge this one.

I also reported missing test coverage of related code in #74 and #75. I think #74 & #75 should be fixed but not block this PR.

@l3ender you can also see that I assigned #55 to you, this is only for our own housekeeping purposes.

I noticed you attached a txt report file. TBH I find the HTML report to be so much easier to understand.

Please accept my apologies for all of these difficulties. My hope and objective is to avoid adding any more untested code if possible.

[l3ender]

I discovered one more surviving mutant in the proposed changes:
...
I think this is the result of the mutant that I reported in #73. I think this mutant should be tested in a separate PR, to resolve #73, before we merge this one.

Please see #76 to resolve #73.

I also reported missing test coverage of related code in #74 and #75. I think #74 & #75 should be fixed but not block this PR.

I can take a look at these this week.

I noticed you attached a txt report file. TBH I find the HTML report to be so much easier to understand.

I generated the Styker report as normal (npm run stryker), but the generated output from the run is in a javascript file, which gets referenced from the html file. Since the html file's contents doesn't change (just the reference to JS file), and since GH doesn't allow uploading JS files directly, I added .txt to the file and uploaded. I realize now that someone wanting to view these results would need to take a lot of steps--do you know of a better way to attach a report (short of hosting it somewhere).

Thanks!

[l3ender]

Seems like the latest build failed due to some sort of timeout--unsure how to resolve this.

[brodycj]

Thanks, I will check it out today. Yeah, something about the build seems flakey. You may want to push an empty commit. -- Sent from my mobile

[erisu]

@brodybits @l3ender don't worry about submitting and empty commit or force push, I already re-triggered the build of the timeout job and it passed. All jobs are green now.

[brodycj]

Thanks @erisu, that was the right thing. I did not have access to my browser earlier today, that is why I wanted to give the author (@l3ender) a way to trigger a rebuild if needed. I would like to avoid having people start closing and reopening PRs just to trigger a rebuild due to the potential confusion.

@l3ender you can already see in PR #76 that it takes care of the remaining mutant. I will now approve & merge this PR, and then regenerate the report from Stryker.

[brodycj]

As I said in PR #76, I manually verified that it does cover the last surviving mutant that I had found. I am now approving with response to some recent points and a few minor comments.

[...] #74 & #75 should be fixed but not block this PR.

I can take a look at these this week.

That would be awesome if you have time for it, thanks.

[...] do you know of a better way to attach a report (short of hosting it somewhere).

My response is coming in PR #61.

In general, I think it would be nice if we could find a way to cleanup both the source code and the tests, just raised #78 for purposes of tracking and discussion.

I will now merge this PR and regenerate the Stryker report.

[brodycj]

I just regenerated the Stryker report for PR #61 and verified that the changes have proper test coverage. One more comment I noticed after merging: I think that new code should use const & let rather than var, adding this to cleanup issue #78.

In terms of updated Stryker reports, I am now thinking we could host on Netlify and post from the automatic Travis CI build. Stryker has a "dashboard" feature on its website but it seems to be limited to badge support right now. I will discuss this further in PR #61 and maybe in a new, upcoming issue (someday).

[Huy-Vu]

Hi guys, I am trying to create watch app with corodva app. Is there any place I could have a document/instruction on how to add watch2 apps and app extensions with cordova-node-xcode? I am sorry but don't know where is the correct place to ask this question. I greatly appreciate any help.

[brodycj]

Please raise request for documentation in a new issue, not in a closed issue or pull request. Thanks.