Skip to content

Test existing WatchKit support #71

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

Merged
merged 3 commits into from
Oct 18, 2019

Conversation

brody4hire
Copy link

Tests originally proposed by @l3ender in PR #56, adapted to test the existing WatchKit support functionality.

I think this testing should be done before we add WatchKit 2 support, as proposed in PR #56.

Here is the report from the Stryker mutator tool:

Note that WatchKit extension functionality in filetypeForProducttype (internal function in lib/pbxProject.js) is still not covered by these updates (see issue #70).

/cc @l3ender - review and feedback would be appreciated

@brody4hire brody4hire added the bug label Oct 17, 2019
@l3ender
Copy link
Contributor

l3ender commented Oct 18, 2019

A few things I'm not positive on:

  1. Regarding watch extensions: adding WatchKit2 extensions automatically adds the extension as a target to existing WatchKit2 app targets (if they exist). I don't see a similar test for original WatchKit, but admittedly I'm not sure if they are linked the same.
  2. WatchKit2 apps are bundled into the products directory of the main app target (see target type and copy files phase). This was the main thing I wasn't sure on how original watch apps functioned as compared to watch2 apps, but not having an original watch 1 project I'm not quite sure on how to validate. Given that the existing watch app target is wrapper instead of products, it must work slightly differently.

Having said that--and also commending the efforts for adding needed coverage for watch1--I don't believe extensive effort would need to be given for something that can now be considered legacy support. Since Apple has moved fully to watch2 support (I can't find any way to create watch1 apps in new projects), I think the current tests are more than adequate.

Thanks!

@brody4hire brody4hire marked this pull request as ready for review October 18, 2019 03:34
@brody4hire
Copy link
Author

Thanks @l3ender. The only purpose of this PR is to test existing functionality (for Watch 1) which is related to the new functionality (for Watch 2) proposed in #56. You can see that I am basically using most but not all of your test cases from #56 to test the existing functionality.

My purpose is in several parts:

  • ensure test coverage for existing behavior (for Watch 1) which would be related to the new behavior for Watch 2
  • avoid breaking existing behavior that is related (for Watch 1), if possible
  • consistent testing for existing behavior and new behavior, as much as possible

If we merge this PR, it may imply that you have to move some of the WatchKit 2 tests that you added in PR #56 (or rename some files that you added).

Does this answer your questions?

Do you think this PR is ready to be merged?

Would you mind taking a quick look at the changes I made, which are in separate commits, to ensure I didn't add anything harmful?

@l3ender
Copy link
Contributor

l3ender commented Oct 18, 2019

Your changes look good and I think this is ready to merge. After that's done, I can rebase the watch2 branch and move watch2 tests into separate files.

@brody4hire
Copy link
Author

Thanks, merging now. And thanks for all of your help with the development in general.

@brody4hire brody4hire merged commit 3588e16 into apache:master Oct 18, 2019
@brody4hire brody4hire deleted the test-existing-watch-kit-support branch October 18, 2019 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants