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

Allow to pass the danger instance and the file path #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Panajev
Copy link

@Panajev Panajev commented May 26, 2020

Allows the convenience API to maintain the same contract as the regular initialiser, but pass the JSON file instead of the stringified JSON object.

The regular initialiser has default values for the Danger instance as well as the result filter allowing you to pass either of those arguments or take the default value, while the convenience initialiser forces you to use a new instance of Danger if you want to specify the name of the xcpretty-fied Xcode build logs .json file.

@DangerXCodeSummaryBot
Copy link
Collaborator

DangerXCodeSummaryBot commented May 26, 2020

Messages
📖 Executed 14 tests, with 0 failures (0 unexpected) in 0.223 (0.235) seconds

Generated by 🚫 Danger Swift against 38f211f

@Panajev
Copy link
Author

Panajev commented May 26, 2020

@f-meloni what do you think of the PR?

Copy link
Owner

@f-meloni f-meloni left a comment

Choose a reason for hiding this comment

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

Thank you for the PR :)
the change itself is ok, but I'm curious of the reasons behind it

@Panajev
Copy link
Author

Panajev commented May 28, 2020

Hello @f-meloni , short story version I have been playing with danger-swift (migrating from danger ruby) and it was a loop of facing a problem, then trying to work around it, facing another problem, trying to work around it, etc... and while currently still facing an issue (will open its separate issue here) with danger-xcode-summary I noted that the convenience initialiser kind of broke convention, in my opinion, with the standard initialiser linking passing the file with the Xcode output in JSON format with forcing to use a new instance of Danger as it would be created inside the method. This way you can still use this plugin the same way as before and it will still create the new instance, but it is a bit more flexible.

...the longer story version with background sprinkled on top? I have been spiking a SwiftPM repository with internal Swift package modules to split our current app back into modules (it used to have lots of CocoaPods used both as private remote pods and development pods as submodules in a template app).

Instintinctively I started with danger-ruby and all the junit, swiftlint, swiftformat, and xcov plugins they offer and it works alright (gives me code coverage data, fails if the unit test fail, shows me which unit test fail, etc...). The problem is that it forces me to create an Xcode project (which swift package supports) and to run xcodebuild (or better just use fastlane scan to run the tests, etc...).

Still, it does not beat the simplicity of:

swift test --enable-code-coverage

Plus, I liked the idea of using a Swift based tool using Swift syntax for a SwiftPM package :).

The problem is that it does not fully work for me. I can get it to run, run the tests, gather code coverage (not sure why it gathers coverage of the test itself too not just the class under test), save errors and warnings from Xcode's logs, etc.. but it will not show the list of unit tests ran nor it tells me how many passed, and worst of all currently the danger-swift based solution does not fail the build and show the failing tests if I intentionally make a unit test fail albeit I can see in the CI or the lcoal logs that the test failed (tried both purely with swift test and xcodebuild test, so both the macOS and Linix travis .yml approaches I saw in this repo).

Anyways, this last bit will end up in a GitHub issueas it is not the place for this in the PR I think :).

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.

3 participants