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 related test targets as testables #818

Merged
merged 5 commits into from
Dec 23, 2019
Merged

Add related test targets as testables #818

merged 5 commits into from
Dec 23, 2019

Conversation

vytis
Copy link
Collaborator

@vytis vytis commented Dec 19, 2019

Resolves #569

Short description πŸ“

It should be possible to run tests if target has any test targets.

Solution πŸ“¦

This PR finds all related test targets to the test scheme. It was already done for test targets themselves, but the functionality was extended to include all other targets.

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

  • Added implementation
  • Added tests

Test Plan πŸ› 

Run with app_with_framework_and_tests fixture and check that in generated schemes:

Previous behaviour didn't break

  1. AppTests scheme has AppTests as a testable target.
  2. FrameworkTests scheme has FrameworkTests as a testable target.

New behaviour works as expected

  1. App scheme has AppTests as a testable target.
  2. Framework scheme has FrameworkTests as a testable target.

@github-actions
Copy link
Contributor

@vytis your pull request is missing a changelog!

@vytis vytis changed the title Add related test targets as testables [WIP] Add related test targets as testables Dec 19, 2019
Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this @vytis

Regarding testing you are right it doesn't look like there's a wholistic test for the scheme generator, a few options for us:

  1. create another internal method with a similar signature to generateProjectSchemes except this one doesn't write to disk, but returns the [Scheme] which would allow us to unit test it without ever needing to go to disk (should be faster and much more reliable)

  2. We can make createDefaultScheme internal to unit test that one method

Hope this helps!

testTargets = [TestableTarget(target: targetReference)]
} else if let targetNode = graph.findTargetNode(path: project.path, name: target.name) {
// Find all test targets that depend on this target
testTargets = graph.targets
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a suggestion, we can limit the scope of the search to targets within the same project as opposed to the whole workspace

A few ways we can go about it:

  1. Scan project targets directly
targetNode.project
          .targets
          .filter { $0.product.testsBundle }
          .filter { $0.dependencies.contains(.target(name: targetNode.name)) }
          .map { TargetReference.project(path: targetNode.project.path, target: $0.name) }
// ...
  1. Add another helper to the graph

In graph we could introduce another method:

/// Returns all target nodes at a given path (i.e. all target nodes in a project)
public func targetNodes(at path: AbsolutePath) -> [TargetNode] {
        guard let nodes = cache.targetNodes[path] else {
            return []
        }
        return Array(nodes.values)
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense, I was thinking what could be the performance impact of looping through all targets. Only looking through project targets sounds much better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kwridan I added the helper, might be useful in other places too

@vytis vytis changed the title [WIP] Add related test targets as testables Add related test targets as testables Dec 21, 2019
@vytis
Copy link
Collaborator Author

vytis commented Dec 21, 2019

@kwridan I have added tests and addressed the points you raised. I think it's ready for review 🀞

testTargets = [TestableTarget(target: targetReference)]
} else if let targetNode = graph.findTargetNode(path: project.path, name: target.name) {
// Find all test targets that depend on this target
testTargets = graph.targets(at: project.path)
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 moving this logic to the Graph model? Something along the lines of:

func testableDependants(of: TargetNode) -> [TargetNode]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pepibumur updated!

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.

Just a minor thing but it looks good overall. πŸ‘ good job @vytis

@pepicrft
Copy link
Contributor

Thanks @vytis. The PR looks great on my side. Let's wait for one more review from @kwridan and we can merge afterward.

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

nice work @vytis

Tested it out on a few fixtures πŸ‘

@kwridan kwridan merged commit c75ad0f into master Dec 23, 2019
@kwridan kwridan deleted the make-target-testable branch December 23, 2019 17:18
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.

Scheme for framework should be testable
3 participants