Change the implementation of auto generated schemes #2641

FranzBusch · 2021-03-10T20:06:01Z

Resolves #2639

Short description 📝

This changes the implementation of the auto generated schemes to only pick test bundles that are naming like the target and have a suffix with Tests, UITests or IntegrationTests

Checklist ✅

The code architecture and patterns are consistent with the rest of the codebase.
The changes have been tested following the documented guidelines.
The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
In case the PR introduces changes that affect users, the documentation has been updated.

kwridan

This is a great first code contribution @FranzBusch 🎉

The changes look good! left some minor inline comments (one should hopefully address the test failure). The other CI check is failing on swift format, you can run bundle exec rake style_correct to re-format the code.

Lastly, mind updating the CHANGELOG file to include a new entry for this change.

Thanks

kwridan · 2021-03-11T07:20:18Z

Tests/TuistGeneratorTests/ProjectMappers/AutogeneratedSchemesProjectMapperTests.swift

+                    target: targetBTests,
+                    projectPath: projectPath,
+                    testTargetName: "BIntegrationTests"
+                ),


I believe we'll need to update the test helper testScheme to allow specifying an array of test target names to cater for the new changes - as in this test case B target will have 3 test targets in its scheme.

e.g.

XCTAssertEqual( got.schemes, [ testScheme( target: targetA, projectPath: projectPath, testTargetNames: ["ATests"] ), testScheme( target: targetATests, projectPath: projectPath, testTargetNames: ["ATests"] ), testScheme( target: targetB, projectPath: projectPath, testTargetNames: ["BIntegrationTests", "BTests", "BUITests"] ), testScheme( target: targetBTests, projectPath: projectPath, testTargetNames: ["BTests"] ), testScheme( target: targetBUITests, projectPath: projectPath, testTargetNames: ["BUITests"] ), testScheme( target: targetBIntegrationTests, projectPath: projectPath, testTargetNames: ["BIntegrationTests"] ), ] )

(heh re-examing testScheme I can see how it can be confusing in this context 😅, perhaps a better choice would have been makeScheme)

kwridan · 2021-03-11T07:24:01Z

Sources/TuistGenerator/Mappers/AutogeneratedSchemesProjectMapper.swift

@@ -90,7 +90,7 @@ public final class AutogeneratedSchemesProjectMapper: ProjectMapping {
        } else {
            // The test targets that are dependant on the given target.
            return project.targets
-                .filter { $0.product.testsBundle && $0.dependencies.contains(.target(name: target.name)) }
+                .filter { $0.product.testsBundle && ($0.name == "\(target.name)Tests" || $0.name == "\(target.name)UITests" || $0.name == "\(target.name)IntegrationTests") }


minor: could possibly have a private helper method isAssociatedTestTarget (or something equivalent)

kwridan · 2021-03-11T07:27:09Z

Tests/TuistGeneratorTests/ProjectMappers/AutogeneratedSchemesProjectMapperTests.swift

@@ -30,6 +30,16 @@ final class AutogeneratedSchemesProjectMapperTests: TuistUnitTestCase {
            product: .unitTests,


It's worth adding another test case to reproduce the scenario you described in the issue to verify the fix addresses it :)

e.g.

ATests > [A, B]

I covered it with my first test case to make sure only the proper targets are picked up. Is that enough for you @kwridan ?

sure that should do it

kwridan · 2021-03-11T07:34:55Z

@all-contributors add @FranzBusch for code

allcontributors · 2021-03-11T07:35:04Z

@kwridan

I've put up a pull request to add @FranzBusch! 🎉

This changes the implementation of the auto generated schemes to only pick test bundles that are naming like the target and have a suffix with `Tests`, `UITests` or `IntegrationTests`

FranzBusch · 2021-03-11T14:19:56Z

@kwridan @pepibumur Could you help me in getting this merged. I am a bit lost why the test if failing

kwridan

Thanks for the updates @FranzBusch!

re: the test failure it seems odd, I wouldn't have thought your changes would impact any of the caching features - will run it locally to see what I can find.

kwridan · 2021-03-12T06:43:49Z

Tests/TuistGeneratorTests/ProjectMappers/AutogeneratedSchemesProjectMapperTests.swift

@@ -30,6 +30,16 @@ final class AutogeneratedSchemesProjectMapperTests: TuistUnitTestCase {
            product: .unitTests,


sure that should do it

kwridan · 2021-03-12T07:09:54Z

Locally running the failing test in question passes on this PR for me - seems we have some sort of environment flake 😞

./bin/fourier test tuist acceptance features/cache-frameworks.feature

cc: @tuist/core

laxmorek · 2021-03-12T07:39:29Z

I had a similar issue with cache-frameworks.feature in my last PR. I re-run jobs and it passed. I don't know where was a problem. 🤔

Definitely features/cache-frameworks.feature should no pass "randomly".

FranzBusch · 2021-03-12T10:00:09Z

@laxmorek or @kwridan could somebody re-run the job for me?

fortmarek · 2021-03-12T10:12:44Z

could somebody re-run the job for me?

done @FranzBusch 👍

Hopefully, it will pass now and we shouldfix the test's flakiness soon as it creates a lot of confusion for contributors when they see a test fail when they did not even introduce any changes to that part of the codebase

laxmorek · 2021-03-12T10:33:16Z

It passed now. Definitely we must fix it.

FranzBusch · 2021-03-12T15:14:13Z

@kwridan What do I need to do to get this merged? :)

fortmarek

Thanks @FranzBusch for your contribution!

kwridan reviewed Mar 11, 2021

View reviewed changes

allcontributors bot mentioned this pull request Mar 11, 2021

docs: add FranzBusch as a contributor #2644

Merged

FranzBusch force-pushed the bugfix/autogenerated-schemes branch from 9ae074d to aaf696f Compare March 11, 2021 11:12

Change the implementation of auto generated schemes

f81498e

This changes the implementation of the auto generated schemes to only pick test bundles that are naming like the target and have a suffix with `Tests`, `UITests` or `IntegrationTests`

FranzBusch force-pushed the bugfix/autogenerated-schemes branch from aaf696f to f81498e Compare March 11, 2021 11:17

kwridan approved these changes Mar 12, 2021

View reviewed changes

fortmarek approved these changes Mar 12, 2021

View reviewed changes

fortmarek merged commit 2b7423b into tuist:main Mar 12, 2021

jakeatoms mentioned this pull request Apr 8, 2021

Auto-generated schemes now broken #2796

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the implementation of auto generated schemes #2641

Change the implementation of auto generated schemes #2641

FranzBusch commented Mar 10, 2021 •

edited

kwridan left a comment

kwridan Mar 11, 2021

kwridan Mar 11, 2021

kwridan Mar 11, 2021

FranzBusch Mar 11, 2021

kwridan Mar 12, 2021

kwridan commented Mar 11, 2021

allcontributors bot commented Mar 11, 2021

FranzBusch commented Mar 11, 2021

kwridan left a comment

kwridan Mar 12, 2021

kwridan commented Mar 12, 2021

laxmorek commented Mar 12, 2021

FranzBusch commented Mar 12, 2021 •

edited

fortmarek commented Mar 12, 2021 •

edited

laxmorek commented Mar 12, 2021

FranzBusch commented Mar 12, 2021

fortmarek left a comment

		@@ -30,6 +30,16 @@ final class AutogeneratedSchemesProjectMapperTests: TuistUnitTestCase {
		product: .unitTests,

Change the implementation of auto generated schemes #2641

Change the implementation of auto generated schemes #2641

Conversation

FranzBusch commented Mar 10, 2021 • edited

Short description 📝

Checklist ✅

kwridan left a comment

Choose a reason for hiding this comment

kwridan Mar 11, 2021

Choose a reason for hiding this comment

kwridan Mar 11, 2021

Choose a reason for hiding this comment

kwridan Mar 11, 2021

Choose a reason for hiding this comment

FranzBusch Mar 11, 2021

Choose a reason for hiding this comment

kwridan Mar 12, 2021

Choose a reason for hiding this comment

kwridan commented Mar 11, 2021

allcontributors bot commented Mar 11, 2021

FranzBusch commented Mar 11, 2021

kwridan left a comment

Choose a reason for hiding this comment

kwridan Mar 12, 2021

Choose a reason for hiding this comment

kwridan commented Mar 12, 2021

laxmorek commented Mar 12, 2021

FranzBusch commented Mar 12, 2021 • edited

fortmarek commented Mar 12, 2021 • edited

laxmorek commented Mar 12, 2021

FranzBusch commented Mar 12, 2021

fortmarek left a comment

Choose a reason for hiding this comment

FranzBusch commented Mar 10, 2021 •

edited

FranzBusch commented Mar 12, 2021 •

edited

fortmarek commented Mar 12, 2021 •

edited