Fixes squash merge request for Single Job Mode #369

Open anuragagarwal561994 opened 1 year ago

GitLab has option to configure squash and merge option at three places:

project level, can either be enforced or defaulted to on / off
while create merge request
while merging the merge request

When we are accepting a merge request, if the option to squash enforced is enabled at a project level, we need to pass squash: true while accepting the merge request in the API call or else we get error on GitLab:

Currently the MR is stuck in loop and marge bot crashes. This MR tries to address and resolve the following issues:

https://github.com/smarkets/marge-bot/issues/326 https://github.com/smarkets/marge-bot/pull/297 https://github.com/smarkets/marge-bot/issues/333 https://github.com/smarkets/marge-bot/issues/152

@snim2 can you help review these changes. Thanks

@snim2 can you help review these changes. Thanks

I'll have a look for you later today.

@snim2 thanks, we have tested the main scenario in our test project and it is working. We are checking remaining scenarios.

Also I needed to add test case for the same, but would require some help to understand how it has been setup.

I don't think I have maintainer rights here, but this looks OK to me!

I've also tested this fix just now on one of our private GitLab repos (GitLab SaaS), and I can confirm this fix works quite nicely. ✅

Thanks @theipster, @snim2 would require some help with the test cases, I haven't had experience writing test cases in python, I am trying still. There are few things I need to take care I believe:

in test_job.py, I am mocking project which is making the condition

    auto_squash = (
            self._project.squash_enforced or
            merge_request.squash
        )

as always True, becuase squash_enforced is mocked. This is defeating the purpose for test case test_ensure_mergeable_mr_squash_and_trailers

Is there a better way this can be handled.

I want to add test case for the same by changing the project level always, should I update the mock in this test case. No examples I guess to know how it is done.
I wanted to add more cases in test_merge_request.py but I am getting confused with the flow, a little help I would be able to write the tests then.

I'm neither an expert in Python nor marge-bot, but I can try and give some suggestions. 🙂

Firstly, this may help: I consider the MR-level squash to mean "I want to squash", i.e. this is my choice, regardless of the consequences. This is in contrast to the project-level squash_enforced that means "I need to squash (otherwise the operation will fail)". Note that these are two different things, but are not mutually exclusive.

So, in terms of your questions:

in test_job.py, I am mocking project which is making the condition
auto_squash = (
       self._project.squash_enforced or
       merge_request.squash
   )
as always True, becuase squash_enforced is mocked. This is defeating the purpose for test case test_ensure_mergeable_mr_squash_and_trailers

This is not defeating the purpose. With the introduction of this project-level setting, this simply means there are now additional scenarios to consider:

#	Scenario	Project `squash_enforced`	MR `squash`
1	"I need to squash" and so "I want to squash"	`True`	`True`
2	"I need to squash" but "I don't want to squash" (and suffer the consequence later)	`True`	`False`
3	"I don't need to squash" but "I want to squash" anyway	`False`	`True`
4	"I don't need to squash" and so "I don't want to squash"	`False`	`False`

Scenarios 1 and 3 (MR squash is True) are already covered by the existing test_ensure_mergeable_mr_squash_and_trailers test case (maybe rename it to test_ensure_mergeable_mr_squash_wanted_and_trailers to make this clearer).

For scenario 2, you should add a new test case (e.g. test_ensure_mergeable_mr_squash_needed_and_trailers): configure the mock project squash_option to be always, configure the mock MR squash to be False, and then assert that CannotMerge is raised.

Scenario 4 is the "happy path" where we don't expect any exceptions. You can add a new test case for this if you like. This scenario is not specific to squashing, and I guess it was simply forgotten.

I wanted to add more cases in test_merge_request.py but I am getting confused with the flow, a little help I would be able to write the tests then.

These existing test_accept_* tests follow an approach called behaviour verification, whereby instead of asserting on the output (i.e. return values), it instead asserts on what actions (i.e. function calls) were triggered. This behaviour verification pattern can be useful when it's painful to control the inner behaviour.

So, in our case, instead of asserting on the exact HTTP response, it simply asserts that a HTTP request was made with certain parameters. This is because it's painful to control the HTTP response.

In terms of how to what you need to do:

Firstly, fix the existing tests. The existing tests expect the HTTP request to be made with a dict of 3 entries, but you've now added an extra params['squash'] = None (default) to the dict, so the existing tests will fail. You'll need to update the assertions to reflect the new dict.
Now, add an extra test for each of the possible options that the auto_squash parameter can accept (I'm guessing True, False and None), and set the asserted dict respectively.

@theipster thanks for this detailed explanation. I am currently busy with my work but would surely like to contribute and complete this PR by the end of this month with proper test cases.

@anuragagarwal561994 I had some spare time and so I decided to roll up my sleeves and have a go myself - I hope you don't mind!

Please see https://github.com/anuragagarwal561994/marge-bot/pull/1, which adds some tests and also fixes some pip dependencies (which are unfortunately necessary for running the test suite).

@theipster thanks a lot for doing this, I will review the changes from my end. No mind of course, I am not getting time to sit for this honestly :)

@theipster I have found the changes to be appropriate and I have merged the same.

@snim2 I guess there is a dependent change by @theipster which is included in this MR as well https://github.com/smarkets/marge-bot/pull/370

I suppose we can review and merge the same first and then we can re-review and merge this one.

@snim2 did you get time to review and merge the same?

@anuragagarwal561994 I don't have rights to merge in this repo :)

@snim2 who can help with this?

@snim2 who can help with this?

Someone from Smarkets, I guess, but you'd have to look back through the old PRs and see who merged them to find a username

@nithyashree675 @qqshfox can you help with the same?

smarkets / marge-bot

Fixes squash merge request for Single Job Mode #369