Allow to not attach a debugger to the test process #3813

svenmuennich · 2021-12-10T07:14:05Z

Resolves n/a
Request for comments document (if applies): n/a

Short description 📝

In Xcode's scheme editor a test action can be configured to either attach a debugger to the test process or not:

Currently tuist always sets the configuration to attach the debugger and there is no way to prevent this. This PR adds a new field to TestAction for configuring whether a debugger will be attached or not. The field defaults to true to not change the behavior for existing projects.

Why is this change necessary?

In our code base we make heavy use of assertion testing via Nimble's throwAssertion() matcher. This matcher does not work when a debugger is attached though (at least on Apple Silicon). Hence we want to be able to not attach a debugger to the test process by default to make the tests run smoothly.

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.
In case the PR introduces changes that affect how the cache artifact is generated starting from the TuistGraph.Target, the Constants.cacheVersion has been updated.

pepicrft

Excellent contribution @svenmuennich. Besides the comment that I left, I'd update the CHANGELOG.md to mention that you are introducing this functionality. Moreover, I noticed the option exists in run action. Would you mind adding it there too for consistency reasons?

pepicrft · 2021-12-10T07:26:17Z

Sources/ProjectDescription/TestAction.swift

@@ -64,6 +70,7 @@ public struct TestAction: Equatable, Codable {
    public static func targets(_ targets: [TestableTarget],
                               arguments: Arguments? = nil,
                               configuration: ConfigurationName = .debug,
+                               attachDebugger: Bool = true,


Because by default we don't set the value, the default here should be false. Otherwise it'll be a breaking change for users.

Maybe I am missing something, but before my changes a debugger was always attached implicitly by not setting selectedDebuggerIdentifier here and hence using its default value, which is XCScheme.defaultDebugger. So to prevent this from breaking anything, the default here needs to be true, does it not?

You're right, to ensure it isn't a breaking change the default would need to be true

pepicrft · 2021-12-10T07:29:57Z

@all-contributors add @svenmuennich for code

allcontributors · 2021-12-10T07:30:06Z

@pepicrft

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

pepicrft · 2021-12-10T07:31:15Z

@svenmuennich I made you contributor of the project so that you can push directly your branches to this repository. Doing contributions through forks is very annoying, so we don't want you to suffer from that :P

svenmuennich · 2021-12-10T07:44:47Z

Moreover, I noticed the option exists in run action. Would you mind adding it there too for consistency reasons?

For the run action the logic is a bit more complex, since we already have some logic for this option to treat normal schemes and schemes for app extensions differently. That is, currently the debugger is only attached for normal schemes but never for app extension schemes. I can add the option there too, which would lead to the following behaviour:

scheme type	`attachDebugger`	is attaching debugger
normal	`true`	✅
normal	`false`	❌
app extension	`true`	❌
app extension	`false`	❌

pepicrft · 2021-12-10T07:47:06Z

For the run action the logic is a bit more complex, since we already have some logic for this option to treat normal schemes and schemes for app extensions differently. That is, currently the debugger is only attached for normal schemes but never for app extension schemes. I can add the option there too, which would lead to the following behaviour:

This would be awesome. If you think the behaviour might be misleading for users, we can add a note in the documentation explaining how the debugger is attached depending on the type of scheme. Thanks a lot @svenmuennich

svenmuennich · 2021-12-10T10:10:40Z

Tests/TuistGeneratorTests/Generator/SchemeDescriptorsGeneratorTests.swift

@@ -1441,57 +1626,6 @@ final class SchemeDescriptorsGeneratorTests: XCTestCase {
        ])
    }

-    func test_generate_appExtensionSchemeLaunchAction() throws {


I renamed this test to test_schemeLaunchAction_for_app_extension

svenmuennich · 2021-12-10T10:11:32Z

I added the functionality to RunAction as well.

kwridan

Thanks for contributing this @svenmuennich !

Once we address the small issue this should be good to go 🚀

kwridan · 2021-12-10T10:42:57Z

Sources/TuistGenerator/Generator/SchemeDescriptorsGenerator.swift

+            debuggerIdentifier = ""
+        } else {
+            launchActionConstants = .default
+            debuggerIdentifier = scheme.runAction?.attachDebugger == true ? XCScheme.defaultDebugger : ""


This has a subtle issue - by default many schemes may omit explicitly specifying a runAction (i.e. it will be nil) - as such we'd be disabling the debugger option for them whereas before it was enabled :/

An example of this is the automatically generated -Project workspace scheme.

Suggested change

debuggerIdentifier = scheme.runAction?.attachDebugger == true ? XCScheme.defaultDebugger : ""

if let runAction = scheme.runAction {

debuggerIdentifier = runAction.attachDebugger ? XCScheme.defaultDebugger : ""

} else {

debuggerIdentifier = XCScheme.defaultDebugger

}

It may be worth expanding the tests to cover this case if they don't already.

Good find 😲 I fixed this as suggested and added a test for this 👍

kwridan

thanks for the updates

svenmuennich force-pushed the pickware/test-action-attach-debugger branch from 455934f to c90a3b8 Compare December 10, 2021 07:16

pepicrft approved these changes Dec 10, 2021

View reviewed changes

allcontributors bot mentioned this pull request Dec 10, 2021

docs: add svenmuennich as a contributor for code #3814

Merged

svenmuennich force-pushed the pickware/test-action-attach-debugger branch from c90a3b8 to 4d47a01 Compare December 10, 2021 10:08

svenmuennich commented Dec 10, 2021

View reviewed changes

svenmuennich marked this pull request as ready for review December 10, 2021 10:11

kwridan requested changes Dec 10, 2021

View reviewed changes

Allow to not attach a debugger to the process running or testing an app

5577e33

svenmuennich force-pushed the pickware/test-action-attach-debugger branch from 4d47a01 to 5577e33 Compare December 10, 2021 11:40

kwridan approved these changes Dec 10, 2021

View reviewed changes

laxmorek approved these changes Dec 13, 2021

View reviewed changes

fortmarek merged commit 976ffca into tuist:main Dec 13, 2021

svenmuennich deleted the pickware/test-action-attach-debugger branch December 14, 2021 11:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to not attach a debugger to the test process #3813

Allow to not attach a debugger to the test process #3813

svenmuennich commented Dec 10, 2021

pepicrft left a comment

pepicrft Dec 10, 2021

svenmuennich Dec 10, 2021

kwridan Dec 10, 2021

pepicrft commented Dec 10, 2021

allcontributors bot commented Dec 10, 2021

pepicrft commented Dec 10, 2021

svenmuennich commented Dec 10, 2021

pepicrft commented Dec 10, 2021

svenmuennich Dec 10, 2021

svenmuennich commented Dec 10, 2021

kwridan left a comment

kwridan Dec 10, 2021

svenmuennich Dec 10, 2021 •

edited

kwridan left a comment

-            debuggerIdentifier = scheme.runAction?.attachDebugger == true ? XCScheme.defaultDebugger : ""
+            if let runAction = scheme.runAction {
+                debuggerIdentifier = runAction.attachDebugger ? XCScheme.defaultDebugger : ""
+            } else {
+                debuggerIdentifier = XCScheme.defaultDebugger
+            }

Allow to not attach a debugger to the test process #3813

Allow to not attach a debugger to the test process #3813

Conversation

svenmuennich commented Dec 10, 2021

Short description 📝

Why is this change necessary?

Checklist ✅

pepicrft left a comment

Choose a reason for hiding this comment

pepicrft Dec 10, 2021

Choose a reason for hiding this comment

svenmuennich Dec 10, 2021

Choose a reason for hiding this comment

kwridan Dec 10, 2021

Choose a reason for hiding this comment

pepicrft commented Dec 10, 2021

allcontributors bot commented Dec 10, 2021

pepicrft commented Dec 10, 2021

svenmuennich commented Dec 10, 2021

pepicrft commented Dec 10, 2021

svenmuennich Dec 10, 2021

Choose a reason for hiding this comment

svenmuennich commented Dec 10, 2021

kwridan left a comment

Choose a reason for hiding this comment

kwridan Dec 10, 2021

Choose a reason for hiding this comment

svenmuennich Dec 10, 2021 • edited

Choose a reason for hiding this comment

kwridan left a comment

Choose a reason for hiding this comment

svenmuennich Dec 10, 2021 •

edited