5

First question here (i'm usually on StackOverflow)...not sure if the right place (if not, please vote to move where it belongs).

If we have a dev process using traditional GitFlow approach, what criteria should be met before a Pull Request (PR) is opened? Specifically, if the work requires manual testing/QA signoff before being merged into master.

There are two sides of the fence here:

  1. PR's should not be opened until the work is "ready". (ready = ready for production, pending any code reviews, which should just be based on code quality/standards). Pros: reviewer doesn't need to check if the work is 'ready to be merged'. It's defined by the process. Cons: longer time (e.g waiting for test signoff) before the code is reviewed. PR's are open longer.
  2. Open the PR, even if it's not ready. Basically, allow the QA testing & code reviews to operate in parallel. Pros: code review and testing can operate in parallel. Cons: if issues are found in testing, the code needs to change, even though it has already been reviewed. So reviewing could happen multiple times, and communication needs to be fed correctly between parties.

Thoughts on either 2 approaches, or is there an option 3? What standards/advice is there in this process?

2 Answers 2

5

The guidance that we've always followed in my various scrum teams is developers should get what they've got into testing as soon as possible. It may mean a little rework later on down the line if there are issues found in code reviews or developers haven't completely finished coding, but the earlier developers get feedback on what's been done, the earlier they can fix any issues, and take account of this feedback in any further coding.

You should make testers aware when they aren't getting completely finished output, but if they've been encouraged to think about the benefits of preventing bugs as well as just finding them, they should hopefully be keen to adopt this process.

It isn't always practicable - for example, if you don't have deployment tools that are flexible in which branches they deploy from, but when it is practicable, it's what Scrum and agile are all about - get feedback and get it early.

3
  • Thanks for your answer. Totally valid points. I guess my argument against is the disjointed communication. When a PR is opened, a developer can review the code, but they can't merge it until the testing has completed. Now, how does the developer know it's been tested? a) does someone tell them? b) do they need to keep asking, c) does a tool tell them? Right now we are in option b), which is slightly frustrating and slows down the process for deployment. What approach do you take to solve this problem?
    – RPM1984
    Dec 12, 2017 at 0:31
  • Does the tool allow the tester to add comments to say that the testing has been completed, so that the developer knows? Dec 12, 2017 at 16:19
  • They do, but this isn't fed back into the reviewer. To be specific: we use Trello for Kanban, and GitHub for PR/code reviews. So when the tester marks the card as 'tested', this status isn't communicated to the code reviewer. I guess this can be solved by a better tool or the code creator to just mark in the PR that testing has passed. To sum up: i agree with "developers should get what they've got into testing as soon as possible", but struggling with communication between tester/developer/reviewer, but i guess it's a problem with the tool.
    – RPM1984
    Dec 19, 2017 at 22:12
4

Open the PR, even if it's not ready. Basically, allow the QA testing & code reviews to operate in parallel. Pros: code review and testing can operate in parallel. Cons: if issues are found in testing, the code needs to change, even though it has already been reviewed.

When code review prompts changes, testing has to be redone. This encourages the building of automated tests over manual tests, as then this can happen automatically without any human involvement.

When testing prompts changes, code review has to be redone. Code review is ideally primarily about a 10,000-foot view of how to approach the problem, but it certainly does cover implementation specifics as well. But this is a problem mostly solved by review tools - they'll show you the changes since you last reviewed so you don't have to review everything all over again. Some tools handle this well with rebasing, but if you're using one that doesn't, stick to adding new commits as you work, and then squash them all together at the end if that's what you want.

1
  • Yep, good answer. Valid points. +1
    – RPM1984
    Dec 19, 2017 at 22:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.