Bug Fix for Merging "reenhanced:master" instead of "master"

[simonzhu24]

No description provided.

[simonzhu24]

@codenamev Apologies for sending another diff, but I found a bug last minute when trying to deliver using branch_name reenhanced:master instead of master. It wasn't a blocking issue on deliver, but it was using the wrong base_branch_name (reenhanced:master instead of master) to update and cleanup.

I am also adding self back into the code as a Ruby insurance that the correct scope of the variable is called so we don't have any confusion in the future. Please accept this asap so the bug can be fixed.

[codenamev]

Can you update the tests please? I have another branch (vs-cleanup-new-gh-merge) I've been working on to clean up some other issues I've found as well before we release.

[simonzhu24]

The tests are passing as I didn't add any additional methods or complex
logic that would cause them to fail.

There was no added functionality so I don't believe new tests need to be
added?

On Friday, May 13, 2016, Valentino notifications@github.com wrote:

Can you update the tests please? I have another branch (
vs-cleanup-new-gh-merge) I've been working on to clean up some other
issues I've found as well before we release.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#171 (comment)

[simonzhu24]

@codenamev do you mind patching in my changes and seeing if the issues that you saw still exist? I believe this patch will solve the clean up and deliver issues that were present.

[codenamev]

The tests are there for quality assurance that the methods do what they say. Since we have restructured so much here, it's possible that the old tests didn't cover the new way of doing things. We'll want to first add failing tests to catch the issue you found so that we can ensure any new changes going forward don't affect it as well.

Also, we have a running PR open doing some cleanup before we can release. It turns out that the squash feature in the API is only in preview and requires special headers to access. While we can simple add those headers in, the github_api gem does not currently support the extra parameters (it filters unwanted options out). There is a PR we have open to get that resolved and will need it released in the github_api gem before we can release this as well.

[simonzhu24]

@codenamev I updated the tests.

[codenamev]

It looks like the bug in question is using a new syntax for the branch name owner:base_branch_name? Can you please update/add tests for that scenario?

[simonzhu24]

@codenamev Perhaps I misunderstood you? The checkout and some cleanup commands that were ran after delivery were git checkout owner:base_branch_name which was incorrect as it should have been git checkout base_branch_name. Because some of those commands were left out of the test, I added them in and added some additional tests. Now the code works and the tests pass.

I just checked locally both with force delivering and normally, each with and without cleanup. All 4 test cases work locally and the test coverage reflects this as well.

Please let me know if we can merge this. This solves an immediate problem with cleanup / deploy.

[codenamev]

why do we need to check for :?

[simonzhu24]

@codenamev Line 180 is the same logic as what I am trying to do previously in line 180 - 186. This is just a bit more elegant Ruby Regex Syntax to match the expression in the string after the ":" to the end and if ":" does not exist, simply match the whole string.

There are 2 cases:
"master" => "master"
"codenamev:master" => "master"

Regex supports both (And it is a single line of Ruby code). ^_^

[codenamev]

Ah, I wasn't aware that Github or Bitbucket formatted the head/base labels like that. Let's move this into the GitHub::PullRequest#initialize and BitBucket::PullRequest#initialize methods. We should be able to trust a consistent format wherever else we need to reference these.

[simonzhu24]

Done

[simonzhu24]

The additional tests make sure that each command that was supposed to run executed on the correct branch_name. It should be master instead of codenamev:master since github does not recognize the latter syntax.

[codenamev]

lgtm, thank you 😄

[simonzhu24]

@codenamev np, lets merge this!