Hotfix a mistake in params on retriggered_by_comment method

[srbarrios]

What does this PR do?

Hotfix a mistake in params on retriggered_by_comment method.

As we added that method (to include both ways of trigger a test), The signature of the method was changed, so now we are passing the PR and we need then to specify the pr.number for issue_comments method.

  def retrigger_needed?(pr)
    # we want redo sometimes tests
    return false unless retriggered_by_checkbox?(pr, @context) ||
                        retriggered_by_comment?(pr, @context)

[MalloZup]

@srbarrios do you have a backtrace of the error?

[MalloZup]

i would also like to know why this error popped-up now, since your PR didn't modify that part of code. thx

[srbarrios]

i would also like to know why this error popped-up now, since your PR didn't modify that part of code. thx

Could you connect to IRC a minute please?

[MalloZup]

sorry guys, for merging a PR should i connect to irc? i think we should do it on Github. this is a opensource project.

I'm trying to be transparent and willing to accept the PR, but i want to understand why we need this change.

thx anyways. I don't think engaging on shurtcuts via IRC is a good way for merging PRs in any projects.

( just wrote via commetns or post example here). for the rest i'm thankfull that you send prs.

[srbarrios]

sorry guys, for merging a PR should i connect to irc? i think we should do it on Github. this is a opensource project.

I just wanted to explain you before add the coment, but read the description, is there.

[MalloZup]

@srbarrios @juliogonzalez ok thx. JFYI for next-time, try to have a real run locally before we create a broken gem.

For the rest ok, thx for this fix

[srbarrios]

@srbarrios @juliogonzalez ok thx. JFYI for next-time, try to have a real run locally before we create a broken gem.

For the rest ok, thx for this fix

Yep, sorry, I did the change at last moment and only run the acceptance tests. Which doesn't catch that issue.

[MalloZup]

@srbarrios no worry, we can all fail. but we should not make it as an habit. :smiley:

The tests-acc either need improvements or we need to catch it manually. ( with some stupid checks via cli).

I think maybe testacc is the safest way but even with testacc i just run it manually when do stuff.

Anyways, thx for the fix and the patch. good job!

[juliogonzalez]

@srbarrios @juliogonzalez ok thx. JFYI for next-time, try to have a real run locally before we create a broken gem.

That's what @srbarrios did, but it seems he did not remember when he commited again.

And that's understandable. It can happen to anyone.

Such test should be automated, not manual. Not when we commit, and not when we submit the gem, but at all times. Otherwise each PR can break gitarro.

IIRC, we have something in the past but we removed it in favour of mock tests. Time to bring it back?

[MalloZup]

i'm not sure if we have removed something with mocking test. we might need to check.

[MalloZup]

anyways, yes when making change things move so it can happen to anyone.

opening following up issue about testacc