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

Add target test coverage #82

Merged
merged 7 commits into from Nov 12, 2019
Merged

Conversation

l3ender
Copy link
Contributor

@l3ender l3ender commented Nov 12, 2019

Resolves #75.

@l3ender
Copy link
Contributor Author

l3ender commented Nov 12, 2019

Reran Styker on these changes and there's only one item left in addTarget:

diff --git a/lib/pbxProject.js b/lib/pbxProject.js
index 3097678..d3d05bd 100644
--- a/lib/pbxProject.js
+++ b/lib/pbxProject.js
@@ -1440,7 +1440,7 @@ pbxProject.prototype.addTarget = function(name, type, subfolder) {
     var productName = targetName,
         productType = producttypeForTargettype(targetType),
         productFileType = filetypeForProducttype(productType),
-        productFile = this.addProductFile(productName, { group: 'Copy Files', 'target': targetUuid, 'explicitFileType': productFileType}),
+        productFile = this.addProductFile(productName, { group: '', '': targetUuid, 'explicitFileType': productFileType}),
         productFileName = productFile.basename;

However, from what I can tell, the group and target options aren't used at all in this code path. I'm leaving them alone, but I think the PR is ready given that the method is well-covered now.

A nice side benefit of these changes is that pbxProject.js now has a mutation score in the green (>80%)!

@brodybits

@l3ender l3ender marked this pull request as ready for review November 12, 2019 15:45
Copy link

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

Merging now, thank you!

Your statement about the remaining mutations sounds right. It would be nice if we could take a better look, with maybe some more testing, to see if we can just remove these someday.

Nice to hear that you got the greener score, I will regenerate my report now.

@brodybits brodybits merged commit 8307518 into apache:master Nov 12, 2019
@l3ender l3ender deleted the add-target-test-coverage branch November 13, 2019 02:50
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.

Internal pbxProject.prototype.addTarget function not properly tested
3 participants