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

Adds codeCoverageTargets to TestAction #619

Merged
merged 5 commits into from Nov 4, 2019

Conversation

abbasmousavi
Copy link
Contributor

Resolves #618

Short description πŸ“

Adds codeCoverageTargets to TestAction

Solution πŸ“¦

This property is already implemented in XcodeProj. We only need to assign values to related properties.

Implementation πŸ‘©β€πŸ’»πŸ‘¨β€πŸ’»

It adds codeCoverageTargets property to the TestAction model in manifest and uses it to populate codeCoverageTargets and onlyGenerateCoverageForSpecifiedTargets in XCScheme.TestAction

@@ -161,6 +161,18 @@ final class SchemesGenerator: SchemesGenerating {
var testables: [XCScheme.TestableReference] = []
var preActions: [XCScheme.ExecutionAction] = []
var postActions: [XCScheme.ExecutionAction] = []
var codeCoverageTargets: [XCScheme.BuildableReference] = []

testAction.codeCoverageTargets.forEach { name in
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a unit test to make sure that the buildable references are added to the scheme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -161,6 +161,18 @@ final class SchemesGenerator: SchemesGenerating {
var testables: [XCScheme.TestableReference] = []
var preActions: [XCScheme.ExecutionAction] = []
var postActions: [XCScheme.ExecutionAction] = []
var codeCoverageTargets: [XCScheme.BuildableReference] = []

testAction.codeCoverageTargets.forEach { name in
Copy link
Contributor

Choose a reason for hiding this comment

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

What about extracting this into a method:

func testCoverageTargetReferences(...) -> [XCScheme.BuildableReference] {

}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Hey @abbasmousavi, this PR is looking great. Besides the comments that I added, I'd add a lint check to make sure that the targets that have test coverage enabled exist in the project. This will be your friend for that.

@abbasmousavi
Copy link
Contributor Author

abbasmousavi commented Nov 2, 2019

Hey @pepibumur, thanks for comments and hints. I have added linting check and a test.

@pepicrft
Copy link
Contributor

pepicrft commented Nov 2, 2019

Hey @pepibumur, thanks for comments and hints. I have added linting check and a test.

Thanks for adding those. There are only 2 minor things missing, updating the documentation, which is in the docs directory, and update the CHANGELOG.md to include this change.

After that we are good to merge this PR.

@abbasmousavi
Copy link
Contributor Author

abbasmousavi commented Nov 2, 2019

Hey @pepibumur, thanks for comments and hints. I have added linting check and a test.

Thanks for adding those. There are only 2 minor things missing, updating the documentation, which is in the docs directory, and update the CHANGELOG.md to include this change.

After that we are good to merge this PR.

@pepibumur, The PR already contains changes to documents and changelog

https://github.com/tuist/tuist/blob/1f5df61c5b24b8ae247ced29dbd25f517c7d7a26/docs/usage/2-projectswift.mdx

https://github.com/tuist/tuist/blob/1f5df61c5b24b8ae247ced29dbd25f517c7d7a26/CHANGELOG.md

@pepicrft
Copy link
Contributor

pepicrft commented Nov 4, 2019

Sorry, I missed those. All good then @abbasmousavi. I'm mergint the PR... πŸš€

@pepicrft pepicrft merged commit 2b40a38 into tuist:master Nov 4, 2019
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.

Ability to specify targets to gather coverage info for
2 participants