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

Pass use_no_ff_batches into merge options #259

Merged
merged 1 commit into from
May 26, 2020

Conversation

mathyoudawson
Copy link
Contributor

#256 introduced an error where the use use_no_ff_batches option was not passed into the
options correctly. Merge options are passed into the batch job class,
and use_no_ff_batches was not included in the merge options.

Thus creating the error:
AttributeError: 'MergeJobOptions' object has no attribute 'use_no_ff_batches'

This PR adds use_no_ff_batches to the merge options.

Previously the use use_no_ff_batches option was not passed into the
options correctly. Merge options are passed into the batch job class,
and use_no_ff_batches was not included in the merge options.

Thus creating the error:
AttributeError: 'MergeJobOptions' object has no attribute 'use_no_ff_batches'

This commit adds use_no_ff_batches to the merge options.
@mathyoudawson mathyoudawson force-pushed the fix-no-ff-batch-merge branch from d25ac18 to a0c87b0 Compare May 12, 2020 02:33
@RobertKirk
Copy link
Contributor

Do you know why the tests you wrote for #256 didn't catch this error? It would be good to cover that, so we can't have a regression.

@mathyoudawson
Copy link
Contributor Author

mathyoudawson commented May 14, 2020

The merge_batch method i introduced was tested, but the caller was not. I was surprised that other tests for the accept_mr method did not pick this up. Will have a go at adding some tests for this :)
It also looks like the happy path for accept_mr is untested.

@RobertKirk
Copy link
Contributor

Great, thanks. It's possible the caller's test mocks the merge_batch method.

@mathyoudawson
Copy link
Contributor Author

@RobertKirk Sorry this has taken me ages.
I've been struggling to wrap my head around how these mocks work.

Every time accept_mr is called during testing the exception Someone pushed to branch while we were trying to merge is raised.
This is due to actual_sha being bound to a mock object <MagicMock name='mock.rebase()' id='4656111568'>
https://github.com/smarkets/marge-bot/blob/master/marge/batch_job.py#L139

Python is not a language I know very well so I feel some of this is going over my head. Would appreciate some help or a push in the right direction

@RobertKirk
Copy link
Contributor

I think looking at it again I'm happy to just merge it in. I agree the mocks are confusing.

@RobertKirk RobertKirk merged commit ca64531 into smarkets:master May 26, 2020
@mathyoudawson
Copy link
Contributor Author

@RobertKirk Cheers for merging this. For what it's worth the company I work for has been running the above code in batch mode for the last week with no issues.

@mathyoudawson mathyoudawson deleted the fix-no-ff-batch-merge branch May 26, 2020 23:51
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.

2 participants