9

Does code-review typically involve UAT/QA testing? We are trying to establish a meaningful code review process and we have come to a disagreement among our development team. Some of are holding that a reviewer should checkout a development branch from TFS, and actually perform UAT based on the acceptance criteria of the User Story (our unit of review) and some are saying that the responsibility for whether the code is functional is, and should remain, on the developer.

What is your experience? Is there an industry standard regarding this question?

Improve this question
3
  • 5
    Insisting on acceptance tests during a code review seems incredibly counter-productive to me. Acceptance tests and code review have two entirely different purposes, so there's no good reason to tie them together, unless you want to deliberately slow things down.
    – Robert Harvey
    Mar 21, 2017 at 17:45
  • If your team has 'junior' level folks hoping to learn from the 'grognards' you should cover the AC in the code review.
    – Matt Balent
    Mar 21, 2017 at 18:01
  • 1
    related (possibly a duplicate): Should code reviewers test as part of the review?
    – gnat
    Mar 21, 2017 at 19:45

7 Answers 7

Reset to default
10

Typically, no. In my experiences, code review happens after any developer-driven testing. The responsible developer would make the changes to the code, update any unit or integration tests, and usually run the code in a local environment if that's possible. Once the changes are made and at least some basic tests around the changes pass (either manual or automated), the changes can be submitted for a code review.

Should your process allow for quality assurance or user acceptance testing as part of a code review? Thats up for you and your organization to decide. Seems like it adds a lot of overhead. There is a difference in what the different types of testing look for. It's relatively easy to run some automated unit tests, see that tests pass, look at a diff of code, and then see that the automated tests provide appropriate coverage, only turning it over to quality assurance or users if two developers understand the change and say that the changes meet the need.

Improve this answer
1
  • I completely agree with the process you described. However, in practice, I saw many reviewers (including myself) test small pieces of the new code, in order to be sure of its consequences. But these are exceptional cases, not common ones.
    – Radu Murzea
    Mar 22, 2017 at 12:48
5

I am not a developer, but an agile coach. So my answer comes from that perspective. There is a logical way to think about your question that should answer it, without needing a development background. I hope you will humor me. Typically, we see a developer work on a piece of functionality, let's say it's in Java, and he or she writes the code and tests (unit, perhaps TDD) and validates that the code works and meets the requirements (or acceptance criteria). At that point, another developer, commonly more senior, performs a code review. At this time, the analysis is on Structural Quality and is enforced or validated via the code review. The review looks for numerous deviations from code standards, or even "transcendental" perspectives on code quality, which is hard to define and gets a bit "artsy". The developers agree on changes to be made to improve the code based on these considerations, they are done, the developer again runs his test and validates that the changes that resulted from code review didn't break the functionality. At this point, Functional Quality of the code is validated through QA and UAT.

If you change the order, and have QA and UAT spend their time on testing, and then perform code review and make changes to the code, wouldn't you need to have QA and UAT yet again validate that everything meets their specs?

You could take a short cut and assume nothing changed, but if you ever broke something you didn't detect in your tests, it could be a big problem. And if you ask them to repeat their testing again, that's highly inefficient and would not be appreciated.

So no, I have never seen any of my teams do it that way, and I've been working with at least a dozen or two agile teams for 8 years.

Improve this answer
1
  • I think there's a slight misunderstanding here. The OP doesn't want to swap QA with code-review, he's asking if the person who is doing the code-review should also personally test the new functionality.
    – Radu Murzea
    Mar 22, 2017 at 12:33
3

To add to the 'no's, I'd add that the name 'Code review' rather gives it away- you are reviewing the code, not the application. Adding testing to the brief can actually be detrimental- if I, as a developer, have to both include tests as part of reviewing the code, a lot of my focus will be on the test results rather than checking the code from the codes perspective properly. This means any currently non-symptomatic problems are far more likely to be missed.

Improve this answer
1

I think purpose of code review is just get extra pairs of eyes on the written code.

Reviewers assume that developer who wrote the code have enough skills for that task. If it is not so, then something wrong in your organisation and any process or tool will not help you.

So to keep reviews fast and simple reviewers will check for possible mistakes which can be overlooked by developer: typos, code styling, does code follow programming guideline of your team, best practices and based on the size of the task - design decision.

It is important that review is not big.
In your case, when you review user story one review can take days or if you organise review meetings it will takes hours of not productive time for developers.

Improve this answer
1

Typical process (for us, anyway) is

  1. Developer codes the feature point or bug fix, as well as any automated tests
  2. Developer performs a local build
  3. Developer performs local unit testing
  4. Developer submits for code review
  5. Lead developer and/or peer developer reviews the code
  6. Developer checks code and automated tests into code repo
  7. CI server detects code changes and pulls latest code
  8. CI server generates a build and executed automated tests again
  9. At regular intervals, builds are installed on a QA environment
  10. QA performs functional testing

Since the code review occurs before the code checkin, it would be unusual for QA to perform any testing concurrent with the code review, since they would have to testing an unofficial build. Generally speaking they are going to want to stick to testing actual builds.

Improve this answer
5
  • Upon further reflection, one thing I am not clear on is: is this an agile team or a waterfall team? In waterfall, Yes, everything stated makes sense. If agile, there is still validity but typically we encourage teams to eliminate the mini waterfall stages and role-based thinking--anyone can test. So if it is an agile team, a developer certainly could test for completion of acceptance criteria as part of the code review, but then why would you need a QA to test it later? Contrarily, if the code review requires changes, user testing would have to be redone after refactoring. IMHO
    – Curtis Reed
    Mar 24, 2017 at 13:49
  • Hmmm... do you even need a software tester on your Agile team?
    – John Wu
    Mar 24, 2017 at 15:17
  • technically, no, QA is not a recognized role in Scrum. But I don't know what their process is, so I can't assume there is not a QA role in their teams. In most of the companies where I have worked, they still have testers. Your question is excellent and really is indicative of the more advanced agile teams, where testing is a skill, and an activity, but not a role. However....note that he specifically calls out "UAT/QA testing", which led me to believe there is something else going on.
    – Curtis Reed
    Mar 27, 2017 at 20:51
  • When I see "UAT" I assume that a developer cannot be a "U". The purpose specifically of UAT is for stakeholders (chickens, not pigs) to accept a release based on overall fitness for purpose. But QA, I suppose, is different, and could be done before or in parallel with a code review, if QA is a skill and not a role.
    – John Wu
    Mar 27, 2017 at 20:54
  • 1
    Agreed, the developer is not the "U". :) I agree with your statement, but that would not be UAT/QA then as was originally stated. Really it should perhaps be stated as "functional testing", wouldn't you agree?
    – Curtis Reed
    Mar 27, 2017 at 20:58
0

No, Typically code review is just a coding style check.

However, I think your question cuts to the heart of the problem ie. what should a code review cover?

When I review code I want to be able to say 'yes I certify that this code works' but in reality that means I should be fully testing that code, effectively doing a full QA job on it.

In reality all I can do is say 'yes this code matches our coding standards document'

Maybe if I have been working on the software long enough I will know a few 'gotchas' that I can check for.

Also I can do a mental check for edge cases and missed requirements. eg. 'when this is null it will error' and 'the code works, but wont scale'

But those are really failings in the requirements or prioritisation of technical debt. Using a code review to address them isn't very effective.

So I would say DON'T do this kind of code review. Expand it out into a check that the requirements cover performance, technical (automated tests, logging etc) and edge cases and QA on those requirements.

Improve this answer
0

I would say no, but it is based on a few assumptions. The developers are probably some of the highest paid employees on the team which makes their time important and costly. If there are other automated tests and/or people who are testing the code, why would a developer waste time reviewing coding problems that could potentially be caught by those processes? Sorry, I'm not running unit tests on someone else's code.

There are benefits of the code review process to both the writer and reviewer of the code. The review process doesn't always address some of the things the other types of tests should catch, so there's no reason to do a code review, before the other tests. I just don't see the benefit of having the reviewer do the extra work. Getting buy in and setting aside time for reviews is tough enough without heaping any additional responsibilities.

Improve this answer

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.