#917 Rultor should not ask architect to confirm merge on a closed PR

[dskalenko]

Rultor should not ask architect to confirm merge on a closed PR #917

[alex-palevsky]

@dskalenko Thanks, I will find someone to review this PR soon

[alex-palevsky]

@carlosmiranda could you review this pull request

[carlosmiranda]

@dskalenko looks pretty good, thanks!

[carlosmiranda]

@rultor merge

[rultor]

@rultor merge

@carlosmiranda Thanks for your request. @original-brownbear Please confirm this.

[original-brownbear]

@dskalenko it seems to me like this is way too big a change for a very small issue, but maybe I'm missing something :)

To me it looks like we have exactly one use case of QnIfPull in our production code as well as one use case of QnIfOpen. So in essence we're simply interested in whether or not something is an open pull request or not. Why don't we simply move the logic in QnIfOpen into QnIfPull.
Unless I'm missing something here we're essentially adding a new class for exactly one if statement.

QnIfPull really is extremely short itself, unless I'm missing something here, we should simply move the logic for the closed PR case in there.
Then we still get the nice simplification in QnMerge you added, without adding more needless complexity here.

[original-brownbear]

@dskalenko these all make sense, but do we actually have a test reproducing the issue anywhere in the new code?
I mean we should have one, where a non-architect asks to merge right?

[dskalenko]

@original-brownbear
we don't have test reproducing the issue, and I agree with you, that would be nice to have it. And it should be like integration test for all chain (with using Agents.java) to correctly cover this case. But I guess that integration test it's not the scope of this issue , and if we want to have it we should create new ticket. Correct me if I am wrong.

[original-brownbear]

@dskalenko a test for a bug is never out of the scope of the issue, see here: http://www.yegor256.com/2015/06/22/valid-reasons-to-reject-bug-fix.html#it-doesn-39-t-reproduce-the-issue .

[dskalenko]

@original-brownbear
Could you confirm please that I understood you correctly, so you propose to move logic from QnIfOpen (which I added and get rid of it) to QnIfPull?

[original-brownbear]

@dskalenko please see my remark about the test in the above first.

Look the issue here is this:

The ticket asks for
Don't ask the architect to confirm on closed PRs.

You created

A huge change-set refactoring the recognition of whether or not a PR is open and tests for that.
No tests for the actual issue that this came from.
Please see this section too in regards to that:
http://www.yegor256.com/2015/06/22/valid-reasons-to-reject-bug-fix.html#it-is-too-big

If you thought that this was too much work for this one issue, you should have started from the test not the fix. I simply cannot merge a fix that has not proper tests and deploy it to production.
But I can merge a sound test that is ignored for now and have the next guy handle the Puzzle asking to fix it :)

---

At any rate, the logic you created appears sound so don't worry :)
Simply merge the logic to the QnIfPull and reuse the tests we already have for PRs and add a case running on a closed PR. This shouldn't be a big thing to do given that you already understand this functionality well apparently :)

[dskalenko]

@original-brownbear
Thanks I understand your point :)
I will revert all my changes and will create test as you asked (can't understand how you still don't have test for Agents.java or may be I just didn't find it, because as you said any changes in this class it's big risk), and will create Puzzle for fix.

[original-brownbear]

@dskalenko any news here ?

[dskalenko]

@original-brownbear
I faced with problem, that very difficult to write test for this issue. This why I spent a lot of time. Finally I wrote it. Could you please take a look.
P.S : I didn't rollback my changes, I just fixed your remarks and added IT case for this issue, hope it good enough

[original-brownbear]

@dskalenko now we're at about 1.2k lines changed after I specifically said this :(

Please see this section too in regards to that:
http://www.yegor256.com/2015/06/22/valid-reasons-to-reject-bug-fix.html#it-is-too-big

Please read that paragraph carefully. After you did, please tell me, do you see any way to reduce the scope of this change? Is all the refactoring you did actually necessary to resolve this issue ?

[dskalenko]

@original-brownbear

Please read that paragraph carefully.

Thanks a lot, I have read it a lot of times and I see the point :)

I simply cannot merge a fix that has not proper tests and deploy it to production.

and

now we're at about 1.2k lines changed after I specifically said this

Basically I have changed not a lot of logic, most of my changes are tests which in my opinion keep the code safe. If you think any of them are redundant I can simply remove it, but I want to point out that I've created IT case which really test something. (I didn't find anything similar before)

After you did, please tell me, do you see any way to reduce the scope of this change? Is all the refactoring you did actually necessary to resolve this issue ?

I don't want to waste your and my time, please provide concrete actions for me to do

[original-brownbear]

@dskalenko

Small preamble :)
The fact that those tickets are tagged with 30min is specifically intended to prevent this kind of situation and mainly to protect you from wasting your time!.
I fully understand that this kind of initiative is generally well appreciated and on a personal level I do appreciate it. But here it obviously creates a tricky spot for me, I will now have to put in a lot of effort into figuring out the details of this PR. Your task here is to provide something that passes as "30 minutes of work by a competent developer". If you do less work than what is acceptable for those 30 min, I will easily be able to point this out, you'll add a little more work and I'll happily merge your changes.
If you do a lot more work than that, you worked for an extremely low hourly rate. Same goes for me though having to review this :) Worst of all though it's always a danger to the stability of the product if this kind of thing is merged. Even if it pertains to the tests, unstable tests are also something to badly avoid for example. Please understand also, that if your PR introduces some major bug, no one will bring this up with you ever! This not happening is my responsibility and mine alone. The moment this thing gets merged, your responsibility ends :)
=> try to keep this in mind for next time please, it's going to help literally all parties here. Especially though it will help your hourly ;)

At any rate I'm on it reviewing it now.

[original-brownbear]

@dskalenko what's the point of adding this method if we already have the method isPull on the issue ?

[dskalenko]

@original-brownbear
I was trying to avoid PMD warning suppress

[original-brownbear]

@dskalenko what warning ? Can you please give the specific warning you're not getting by using this but getting when directly using the issue methods ?

[dskalenko]

@original-brownbear
sorry didn't specify
Avoid if (x != y) ..; else ..; (ConfusingTernary)

[original-brownbear]

@dskalenko just flip the if and else then.

if(x!=y) {foo} else {bar} => if(x==y){bar} else {foo}

[original-brownbear]

@dskalenko this suppression is wrong, please don't use it and rather use local variables/constants properly.
Also why 500 lines ?

[original-brownbear]

@dskalenko please see a number of comments.
Also please update the PR description to clearly explain what is in this PR in terms of changes, not just what it is supposed to solve. See #1031 as an example of the level of detail that is reasonable here.

[dskalenko]

@original-brownbear
Common answer regarding why I have added mock classes :
I did this because as I said very difficult to add IT test, Mk* classes don't allow me to pass thought all chain and do for example merge command. For example I can't pass thought QnFollow because MkGithub :

public Request entry() {
return new FakeRequest()
.withBody("{}")
.withStatus(HttpURLConnection.HTTP_OK);
}

always return HTTP_OK, but I need HTTP_NO_CONTENT according to QnFollow logic.
The same for MockIssue I "Overrided" public JsonObject json() to be able pass through QnIfPull (issue.isPull()). All other mocks were added just for linking.
Hope you understand the idea. If you have another idea how I can do it, please provide it to me

[original-brownbear]

@dskalenko why do you need to change the agents behaviour in the first place ?

Honestly if a fix this small requires an additional few hundred lines of tests this leaves exactly two possible explanations:

• either the code is extremely bad, in this case creating a proper ticket explaining why that is so is in order.
• the fix does more than fix the issue

The later is evidently the case here, given that you needed to adjust code around handling a merge command posted in a non-pr.
Look I mean there is so many ways of addressing this. Eventually though it always comes down to the same:

The String you don't want to show is only used in com.rultor.agents.github.qtn.QnByArchitect#understand. We should simply have to write a test for this class testing that.
But unfortunately you refactored the order of method calls moving the QnByArchitect inside other calls way altering behaviour and now requiring a different test framework than the one we have.
Why can we not just restore the logic with QnByArchitect as the outer most function and fix it from there?
This would instantly resolve the need for hundreds of lines of new code! I mean we can't just do a huge refactoring and then add a bunch of duplicate test code on top because we break the usefulness of existing test functionality.

[dskalenko]

@original-brownbear
I initially considered your proposal to partially copy logic from QnIfPull and QnMerge to QnByArchitect which introduces code duplication and violates single responsibility principle. I think it looks very ugly and my solution is simple and effective.
If for you better to have less lines of changes than adequate solution, then we are speaking on different languages (besides issue owner agreed with my solution).
I am not ready change my coding principles at this moment and push low quality fix for this issue.
I see two solutions :

1. stop working on this issue
2. to have constructive dialog about my solution instead of counting lines of code

Please let me know how we should proceed

[original-brownbear]

@dskalenko the issue is extremely simple here, and I've been trying my best to bring this across ever since looking into this (also it's explained in detail in the link I posted).
This fix is, outside of having a few serious quality issues ( which can be fixed ), simply breaking process here. I fully understand where you're coming from in the above, really.
But the process simply requires a fix to at least ( outside of fixing the issue ):

• not degrade code coverage
  • check
• come with a test reproducing the issue
  • We sort of have this now ... we have a test reproducing the situation mentioned in the ticket, unfortunately it required a change to the overall way the functionality works. So we don't actually have a test reproducing the issue on existing code. Obviously this is not always possible, here it would have been possible though ... without the refactoring of the production code the tests surely would have had more validity/value. (one of the reasons for never fixing an issue while refactoring!)
• Too big ... and I quote:

Create a new unit test, reproduce the bug, and commit it. Fix the bug in the existing code base, no matter how ugly it is. Create new bugs, asking the team to improve the situation with the ugly code base.

• this aspect the provided PR fails entirely. It's not about number of lines changed, it simply changes more code than necessary to fix the issue. Moreover, while this PR could be seen as cleaning up production code slightly, it introduces a high number of issues (that require a lot of explicit warning suppression) into the test code anyways.

So, to bring this to a conclusion one way or another. This fix simply violates our procedures and I quote from the linked article again:

It's not about being lazy and unwilling to fix what looks bad. It's about a discipline, which is much more important than good intentions.

Procedure you may discuss in https://github.com/teamed/xdsd, but here is not the place. Given that this really is a very standard situation this discussion ends here.

You are free to either:

• Provide a fix in compliance with our procedures in a timely manner.
• Ask @alex-palevsky to reassign this and close this PR

[alex-palevsky]

@carlosmiranda 10 mins added to @original-brownbear account (our architect), payment ID is AP-9V874756A5519974C, since quality is good here... thanks, I just added 17 mins to your account, payment 81394671, 333 hours and 49 mins spent... you're getting extra minutes here (c=2)... +17 added to your rating, at the moment it is: +2579