-
Notifications
You must be signed in to change notification settings - Fork 221
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
Default to all targets when plugin --target
parameter missing. Fix #483
#608
Default to all targets when plugin --target
parameter missing. Fix #483
#608
Conversation
I'm a bit nervous about the complexity of that test. Setting up a new SPM workspace, running SPM, and then checking output feels like it's a recipe for flakiness or potential problems if SPM upstream ever changes their behavior. I'm good with the change to the plugin itself, but can you drop the test? I'd like to think more holistically about how to get better coverage there and in other parts of the codebase that have historically been lacking it. |
I've deleted the test. I think half the value in tests is that they make you proof-read your code again and again, so we did get at least that benefit from it. SPM's PluginTests::testCommandPluginInvocation() does exactly that – creates a package in a temp dir from a string and executes the plugin. It also has a Fixtures/Plugins directory which are the projects used in other tests in that class. It's definitely nicer than what I wrote! // Invoke the command plugin that prints out various things it was given, and check them.
testCommand(package: package, plugin: "PluginPrintingInfo", targets: ["MyLibrary"], arguments: ["veni", "vidi", "vici"]) { output in
output.check(diagnostic: .equal("Root package is MyPackage."), severity: .info)
output.check(diagnostic: .and(.prefix("Found the swiftc tool"), .suffix(".")), severity: .info)
} Sources/SPMTestSupport/misc.swift is the set of functions for working with the fixtures. The SPMTestSupport library Maybe all it would take to use it here would be to add this to SPM's Package.swift: .library(
name: "SPMTestSupport",
targets: ["SPMTestSupport"]
), I'll take another look at using it – I'll try symlinking it or checking out and editing SPM – and if it works well for this test case I'll open an issue in the SPM repo to start a discussion. |
The problem with this approach is that depending on that would make all of the I suppose that could be partially solved by making a completely separate test package in a subdirectory, but then you have to remember to run those tests separately since This feels like an area that SwiftPM needs to come up with a good solution for. While it would be nice to have total coverage here, I also don't feel that we should go to heroics if it's not easy to do or would be potentially fragile. (It's easier for SPM to do those full integration tests by constructing packages in a temp dir because if they change something under the hood, that project's own tests would catch it and be modified to support it. It's less easy for a downstream project.) |
Agreed. I hope the SPM maintainers already have it on their mind to spin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! Merging now.
…s-fix-483 Default to all targets when plugin `--target` parameter missing. Fix swiftlang#483
When either of the swift-format plugins is run without the
--target
parameter, an error is returned. This PR implements the fix by @soumyamahunt in the #483 discussion, default to all targets when none is set:Before:
After:
The test works, but maybe there's a BDD library I should be using instead. It sets up a project in a tmp dir with
swift-format
as a dependency, then confirms the error is absent from the stderr output.In the sample SPM project I included one each of executable, library, test, plugin targets. AIUI,
context.package.targets
does not return the plugin target. I.e. as this fix stands, omitting--target
plugin parameter and expecting "all" targets to be linted, does not lint any plugin target.I tried various ways to unit-test the plugin first. Discussion on the Swift forums says it cannot be done! I symlinked the plugin file into the test target and added SPM as a dependency but got blocked by access control levels as I tried to mock a PluginContext.
I also tried to implement the tests the same as SPM's own fixtures tests but didn't manage to get that working either.
With the current setup (write Package.swift to a temp dir, run
swift package ...
in a NSTask Process) I was only able to verify the output of stdout and stderr. I tried a few ways to determine what parameters the swift-format plugin was calling the executable – subscribing toNSWorkspace.willLaunchApplicationNotification
, loopingNSWorkspace.shared.runningApplications
, and an attempt to implementps -ax -o command | grep swift-format
. I couldn't find any way to determine sub-processes of the Process the test was creating.So, this does fix #483, but I'm open to changes to the tests if there's a more conventional way to write them.