Fix pending status using check parameter and add a "skipping comment" in checkbox when the test doesn't apply

openSUSE / gitarro

run all your test against a GitHub Pull request

https://opensuse.github.io/gitarro

MIT License

15 stars 20 forks source link

Fix pending status using check parameter and add a "skipping comment" in checkbox when the test doesn't apply #167

Closed srbarrios closed 5 years ago

srbarrios commented 5 years ago

What does this PR do?

Fix the bug discussed here #166, where gitarro was adding a pending status on PR, when it runs with --check parameter.

In addition, when the test doesn't apply, we'll unmark the checkbox but we will include a comment to tell the user that this test was skipped.

A bit out of the scope, I also fixed something on gitarro.rb, so if we call the command with a -P parameter, we don't need to checkout all the PRs from github and jump in an iterator, as we can just checkout directly the PR passed by param.

I changed version, to save a commit, let me know if you prefer in a different commit.

What issues does this PR fix or reference?

https://github.com/openSUSE/gitarro/issues/166

Tests written?

I improved those related with rerun, so now it lets a cleaner environment

srbarrios commented 5 years ago

Could you give me a hand solving that issue in Rubocop?

lib/gitarro/opt_parser.rb:179:3: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for defaults_false is too high. [7/6]
  def defaults_false ...
  ^^^^^^^^^^^^^^^^^^

I only added a default value for the new flag. I guess we should set the default values differently, but I couldn't found the way, maybe because it was a long day...

Please, take a look, see if we can fix or we just avoid the issue adding a exclude.

MalloZup commented 5 years ago

hi @srbarrios i will have look on this tm. I'm not sure if adding a new flag can fix the problem.

Imho if check is adding status pending, this is not an expected behaviour and check shouldn't add pending. anyways, thx i will have look once i have time tomorrow :rocket:

srbarrios commented 5 years ago

Please read the issue #166 before. I wanted to discuss it with you today, but as you were not available I gain time creating the PR.

Also, I would like to have an agreement on that change with @juliogonzalez.

MalloZup commented 5 years ago

feel free to close this as discussed

srbarrios commented 5 years ago

I will not close, but just change the PR to fix the issue differently, as still some things that still apply.

MalloZup commented 5 years ago

as you wish :robot:

srbarrios commented 5 years ago

@MalloZup @juliogonzalez done. Tests passed. Testing gitarro manually looks good too.

srbarrios commented 5 years ago

@juliogonzalez and me were discuss exactly that yersterday afternoon, it's hard to test it in Jenkins, using the current system to deploy gems. Let me discuss it with him again.

srbarrios commented 5 years ago

After improve our CI to test properly a gitarro PR, I found some things failing, work in progress again.

MalloZup commented 5 years ago

ok feel free to merge if it is ok for you thx