Here are the top 37 things I check for in Code Reviews
All Code Reviews
Code has tests
Linter is being used
English readable code
Lines are not too complex
Typos for spelling and grammar
Methods are short (<= 5 lines is ideal)
Dependencies are mocked for unit tests
Classes are short (< 100 lines if possible)
Debug Statements that were not ...
"Programs should be written for people to read, and only incidentally
for machines to execute".
-- "Structure and Interpretation of Computer Programs" by Abelson and Sussman
UI automation code is just another piece of "code" hence all the code best practices applies in the same way.
If I have to pick one thing, I would say code ...
It's an okay idea with some guidelines. QA code is software. And software engineering principles should still apply. Since none of the developers know protractor, they should be trained. They may be able to add tests to their application on their own eventually.
As for who to code review, the frontend developers are probably a good choice. They might ...
First of all, whether a team member works part-time is irrelevant. Your team has a sprint backlog based on its velocity, which is variable for each sprint (dependent on team capacity for that sprint). If someone works only four hours a day, the workload for that person should be planned accordingly. Within the time allotted, everyone must do a proper job.
How about the definition as on the Wikipedia: Code review is systematic examination of computer source code.
I think the main added value of code reviews is knowledge sharing and not finding defects. That's why I would like to add a bit of more text, how about this:
Code review is systematic examination of computer source code with the goal of sharing ...
Of-course, understanding the goal of an user-story before coding review gives a lot of context. In an Scrum team I would expect all team-members to already understand the acceptance criteria as everyone was part of the Sprint planning session.
Still actively verifying the (user/customer level) acceptance criteria of an user-story does not seem something I ...
The goal is to find bugs while leaving time and money to do other things too. So a high-quality test is a test that finds bugs but doesn't require a disproportionate expense to write, maintain, or operate.
Sometimes you can tell whether a test is low-quality just by looking at it. For example, if the test is buggy or written in a brittle way so that ...
No, test-cases are about behaviour not implementation. The implementation could change, but most often the behaviour would stay the same.
Coupling test cases to implementation would mean you need to change the test-cases every-time you refactor the code. This is an anti-pattern. So don't specifically use test-cases to check how the application was ...
Low quality tests
When I think of low quality tests, I think of teams mis-applying or over-applying a single test techniques in a way that doesn't yield adequate information.
I've provided an example below along with high quality tests.
High quality tests
I don't know of any summarized guidelines, although I could see a lot of value in this.
I am ...
In my personal opinions,
White box testing is made possible by giving testers access to source code, and comprehensive testing techniques such as MC/DC testing, condition decision testing can be applied.
Questions you may need to consider:
Is every tester working with you able to read source code? If they are able to read source code, do they want ...
To answer this, I think it is better to first look into why they have code access restrictions set. The fact that it is financial software is a valid reason. Minimizing risk of a security breach by minimizing exposure to source code is very important. This is maybe not the best comprison, but if during a snow storm, you don't need to travel, don't do it, ...
If the coding standards of the organization require that each element has a clear name and limits the data to be entered, and the test team is typically part of code reviews, then by all means include these checks with your test cases.
If the organization doesn't require controls like this, it's probably better for your interaction with the ...
I like to put test-data into Class objects and not in text-files. This way you can use proper code completion for your test-data. Centralizing test-data from "strings" is a must, you do not want to change hundreds of "strings" when changing test-data.
Encryption of test-data:
Encryption of test data should be a company policy, but ...
Testers having access to the source code and testers doing code reviews are completely separate things. More often than not testers are not that technically competent and also from my experience not even all developers can do code reviews.
Having read access to the application source code is useful for testing, if you are able to read and understand code.
Ofcourse testers should have access to the code that is being made by the developers.
In an Agile development testing should be an parallel activity. If you have dedicated testers they should be able to add automated tests during development and or do exploratory testing on builds they build them selfs from the code.
To quote the Less framework:
You're on the right track towards DRY test code (Don't Repeat Yourself).
This is how I'd refactor what you have:
Data-drive the tests - you have a series of almost identical tests here. For each type of test (login test, create user test), you create a data file - CSV or XML works. There is an example with TestNG here. For the login page I'd do something ...
After a quick review of the code I come with the following comments:
Do not use thread.sleep(), this leads to flickering tests, in this case wait for the element instead, read: https://stackoverflow.com/questions/11736027/webdriver-wait-for-element-using-java
You do not need to use an Action to send an Enter key, read https://stackoverflow.com/questions/...
There is an IEEE standard for formal software inspections etc, which is a special form of reviews: IEEE Standard for Software Reviews and Audits, IEEE Std. 1028-2008
Furthermore, there is a definition of "code review" and "change-based code review" in the scientific paper "A Faceted Classification Scheme for Change-Based Industrial Code Review Processes" (...
It depends on the kinds of tests.
High quality unit tests have the following characteristics
They mock and stub out external dependencies such as the database
They run fast
They test a single piece of functionality
High quality UI feature tests have the following characteristics
They reflect real user usage, actions and workflows
They do not fail ...
I'd highly recommend you take a look at the Airbnb Style Guide. Although, it's a little long and has some subjective rules, you can easily pick out your favorites and apply them to your code reviews.
Also, it's nearly impossible to remember all the JS rules and best practices so while you're coding I'd really recommend you use a linter like eslint. You can ...
No, part time or full time should not affect the quality of the work.
A well defined document with coding standards should solve this, it is never too late.
Any kind of work in any way is done, small or large sprints should have the same standards, not having a quality standard can result in poor work that can lead to more work when later you need to ...
Code style checking should automated with a linter (e.g. ESLint and good style-guide like airbnb). No-one should be allowed to push code unless they ran the linter and fixed the coding linting errors. This should save you a lot of time.
Merge conflicts should be solved by the coder. I would expect before the push the latest ...
Why do you view it as 'Developer' reviewing 'tester's code?
A team member is reviewing another team member's code is perfectly fine as long as
vice-versa is also true and accepted equally.
Having said that, All code is code, where the standard best practices can be applied and reviewed irrespective of any specific framework (Protractor) knowledge.
I am not personally aware of any 'industry' standard.
The best combination of practices that I have observed at several companies that do code review is:
Tools that automatically fix non-code issues such as blank trailing lines, blank trailing spaces, incorrect use of tabs, etc.
Tools such as linters and cops that report on code quality and suggest /...
There are many styles of reviewing. Systems such as Fagan Inspections aim to find most faults. They are very expensive but they claim very good results. At the other end there is "Hey Nigel, have a quick gander at this code please".
Perhaps every project manager has their own view on how to conduct reviews. I believe that most expect them to be quick to ...
Crucible (like user246 suggested) seems like a tool which has the functionality you are looking for. I have used it in the past to review commits and not pull requests. I think you can also assign parts of code.
Alternatively you can have a look at the open-source Review Board which supports multiple ways to create reviews, from commits, hooks or even ...
If you are testing safety, or other, critical code then your testing requirements will include 100% Code and Decision coverage - this is often impossible to achieve without seeing the actual code.
The specifications which you cannot test without are usually easier to understand & steal than the actual code so confidentiality is only an issue if ...
As mentioned in comments, SCRUM is a process/methodology for executing functional work and not in the weeds on what "proper" work is/isn't. That being said there are certain checkpoints that need to be adhered to and I think you are touching on some of those, so this answer is scoped more to the touch points of reviews during SCRUM and not detailed ...
Question together with the team if elements really need names or id's. Just adding them without a real need seems overhead. Apply the YAGNI princible. Do not create extra work if you do not really need it.
Ask the team how they want you to report it, this if you think something might be an issue. Signaling things are missed during code-reviews are good. ...
Report it, and set the priority appropriately. If the missing identifiers or classes or what not are causing difficulty, then it's obviously a higher priority than if it's missing but unused.
Part of QA is to know what's important and what's not, and communicating that out to the broader team.