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

Create undercover-report command #3

Merged
merged 6 commits into from
Oct 20, 2020
Merged

Create undercover-report command #3

merged 6 commits into from
Oct 20, 2020

Conversation

rafayet-monon
Copy link
Contributor

What happened 👀

The previous method to create the report undercover -c $compare_git_ref > coverage/undercover.txt was not so user friendly.

Insight 📝

To fix this we created a new CLI command undercover-report which takes the same arguments as undercover and saves the output in a file for the danger-undercover to read from.

undercover-report -c origin/development

Proof Of Work 📹

Tested the implementation in our internal project and it's working seamlessly 🤩

  • No Changes

image

  • Changes in new commits

image

@andyduong1920
Copy link
Member

IMO I prefer the original once, the differences between each solution is only the > coverage/undercover.txt which I don't think it's big, the undercover-report looks like hiding the output option there.

Also, I guess we can use undercover -c $compare_git_ref without the > coverage/undercover.txt, I mean that could be used in another case, not our case, but that is possible to run just `undercover -c $compare_git_ref, please correct me if I'm wrong.

so

1️⃣ undercover -c origin/development > coverage/undercover.txt

vs

2️⃣ undercover-report -c origin/development

I prefer the 1️⃣ solution

@rafayet-monon
Copy link
Contributor Author

rafayet-monon commented Oct 19, 2020

IMO I prefer the original once, the differences between each solution is only the > coverage/undercover.txt which I don't think it's big, the undercover-report looks like hiding the output option there.

Thank you @andyduong1920. As you know the first initiative was undercover -c origin/development > coverage/undercover.txt. But when running this in semaphore it kept saying you don't have permission to create this file. I guess it was a problem with the root issue which was fixable.
But, I thought if I wanted to implement in other projects as well it was too much work for just a plugin. So I changed it this way. to reduce complexity. Also, I plan to create a -o $output_path option.

Also, I guess we can use undercover -c $compare_git_ref without the > coverage/undercover.txt, I mean that could be used in another case, not our case, but that is possible to run just `undercover -c $compare_git_ref, please correct me if I'm wrong.

Absolutely, we can do that. Everything from undercover is still available. undercover-report just adds one extra layer.

Also, I created an issue in undercover for a -o $output_path option. If they want it this solution can be reverted.

Let me know your thoughts :)

Copy link

@olivierobert olivierobert left a comment

Choose a reason for hiding this comment

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

I don't share the opinion of @andyduong1920 on this as I prefer this version in the sense that it favours convention (using a default path) while still allowing custom configuration to be passed. In the end, it's simpler to use.

let!(:file) { File.join(Dir.getwd, 'coverage/undercover.txt') }

before(:each) do
allow(described_class).to receive(:`).and_return(mock_message)

Choose a reason for hiding this comment

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

What is receive(:)` for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

receive(:)` is to mock the -

undercover #{args&.join(' ')} CLI command output.

Choose a reason for hiding this comment

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

Wow, I would not have guessed. So a comment would help future devs, yourself included.

@@ -23,15 +23,15 @@ class DangerUndercover < Plugin
# If there are reports then it shows the report as a warning in danger.
# @return [void]
#
def report(undercover_path, sticky: true)
def report(undercover_path = 'coverage/undercover.txt', sticky: true)

Choose a reason for hiding this comment

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

One minor improvement for code discovery would be to have a constant for this:

DEFAULT_PATH = 'coverage/undercover.txt'.freeze

def report(undercover_path = DEFAULT_PATH, sticky: true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8bd4983

Copy link

@olivierobert olivierobert left a comment

Choose a reason for hiding this comment

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

Please add some extra comment for the specs. The rest looks good to me.

@rafayet-monon rafayet-monon merged commit 2aed366 into development Oct 20, 2020
@rafayet-monon rafayet-monon mentioned this pull request Oct 20, 2020
@rafayet-monon rafayet-monon deleted the feature/cli branch October 20, 2020 07:41
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.

4 participants