4 ms·

I’d be curious what these super secret tools do. Most “rigorous code reviews” I’ve seen are junior devs fighting over “tabs vs spaces” or “doing a for loop vs a

I’d be curious what these super secret tools do. Most “rigorous code reviews” I’ve seen are junior devs fighting over “tabs vs spaces” or “doing a for loop vs a for each” - or something else borderline meaningless. I’d love a tool that helped with context in a larger system design way. Coming in cold on some project and expecting to know all the design decisions, and if the code structure is going away from some higher level goal is where most problems that I’ve seen come in. I don’t see how any of the current tools help with that, and I somewhat doubt these secret tools do either - but would love to seen them if they do.

rwiggins 4y ago
One random thing I've been impressed with (that I know is public) is mutant testing: https://testing.googleblog.com/2021/04/mutation-testing.html https://testing.googleblog.com/2021/04/mutation-testing.html , https://research.google/pubs/pub46584/ https://research.google/pubs/pub46584/ To pull an example from the paper, if your line of code says if (a == b || b == 1) you might get a comment that says something like > "Changing this line to if (a != b || b == 1) > does not cause any test to fail." Page 4 lists other mutations, like replacing logical ANDs/ORs with just `true` or `false`, switching arithmetic plus to minus, etc. This isn't a larger system design context thing, though, just a testing one. Disclosure: I work at Google.
- technion 4y ago
  There's a pretty good Ruby gem I've used for this before: https://github.com/mbj/mutant https://github.com/mbj/mutant
- YZF 4y ago
  This is for test coverage? I get it technically but not clear to me if this adds value. Covering all the internal states/code paths in tests seems very hard and potentially of diminishing returns? Isn't time better invested in other things? I'm also guessing this ties into Google's tooling of being able to tell which line of code is hit by which tests and running those as part of the mutation test? EDIT: The blog has some discussion about the correlation between some of these mutation failures and real bugs. I remain a little suspicious to be honest. Also "I also looked into the developer behavior changes after using mutation testing on a project for longer periods of time, and discovered that projects that use mutation testing get more tests over time, as developers get exposed to more and more mutants. Not only do developers write more test cases, but those test cases are more effective in killing mutants: less and less mutants get reported over time. I noticed this from personal experience too: when writing unit tests, I would see where I cut some corners in the tests, and anticipated the mutant. Now I just add the missing test cases, rather than facing a mutant in my Code review, and I rarely see mutants these days, as I’ve learned to anticipate and preempt them." Now this one is expected, if you build automated tooling that comments on your code review you expect people to try and avoid that. However there's still the question of what's the quality improvement and what's the productivity impact.
  - gravypod 4y ago
    Production impact: if you can replace: if (a == 10) With if (true) Then you're not testing the behavior of your application. If in production `a != 10` and you've never tested that it might be a problem. Mutation testing can also do stuff like removing entire statements, etc.
    - YZF 4y ago
      The question (well, one question) is why would we suspect the lack of a test indicates a bug? For most non-trivial software the possible state-space is enormous and we generally don't/can't test all of it. So "not testing the (full) behaviour of your application is the default for any test strategy", if we could we wouldn't have bugs... Last I checked most software (including Google's) has plenty of bugs. The next question would be let's say I spend my time writing the tests to resolve this (could be a lot of work) is that time better spent vs. other things I could be doing? (i.e. what's the ROI) Even ignoring that is there data to support that the quality of software where mutation testing was added improved measurably (e.g. less bugs files against the deployed product, better uptime, etc?) Is this method better than just looking at code coverage? Possibly none of the tests enter the if statement at all? EDIT: where I'm coming from is that it's not a given this is an improvement to the software development process. It seems like there was some experimentation around the validity of this method which is good but like a lot of software studies somewhat limited. It also seems there's a lot of heuristics based on user feedback, which is also good I guess, but presumably also somewhat biased. The related paper has a lot of details including: "Since the ultimate goal is not just to write tests for mutants, but to prevent real bugs, we investigated a dataset of high-priority bugs and analyzed mutants before and after the fix with an experimental rig of our mutation testing system. In 70% of cases, a bug is coupled with a mutant that, had it been reported during code review, could have prevented the introduction of that bug." Which should imply(???) that 70% of "high priority" bugs can be eliminated during the review process by using this sort of mutation testing. Seeing data to that effect would be cool (i.e. after the fact) and if it's real that'd be pretty incredible and we should all be doing that.
      - 4y ago
dilyevsky 4y ago
Fighting over style is resolved via eng-wide style guide and not having juniors approving each other’s code ;)
dodobirdlord 4y ago
One thing Google has going for it to make code review more efficient is the concept of a uniform per-language style guide. The style guides are absolute and comprehensive, meaning it’s not permitted to diverge from them, and equally importantly, that any style topics not covered by the style guide are not considered legitimate style issues to raise in code review. Formatting can be almost entirely automated with pre-submit checks that the code is formatted, and automatic formatting tools. So for your two examples, this practice of adopting a style guide prevents them from arising. The “tabs vs spaces” argument is simply settled, because the style guide has an answer. The “for loop vs for-each loop” is either addressed by the style guide, in which case there is an answer, or it’s not, in which case it is not a legitimate style complaint one way or the other, and the reviewer is making a mistake by bringing it up. https://google.github.io/styleguide/ https://google.github.io/styleguide/
- elischleifer 4y ago
  I would say any styleguide that is in prose form and not machine enforced is deficient. Modern linting and formatting tools are the best, most efficeiect means for enforcing style. No religious arguments are needed if the tooling decides what is correct. Shameless plug for https://trunk.io/products/check https://trunk.io/products/check - which will handle universal enforcement of all the tooling for all of the pieces of your tech stack.
- 0x20cowboy 4y ago
  Yeah sorry I was just trying to pull something silly out of the air - we do mostly go and gofmt (or clangd or whatever). I was just trying to flowerly highlight that most “in depth code reviews” I’ve seen are very focused on surface level things. I’d dig a tool that focused more on structural changes.
  - jeffreygoesto 4y ago
    Well. One could argue that if you have to discuss these very deep topics in a PR, you better had brought them up before coding that stuff wrong in the first place. Oh, that is not considered agile you say? Discussing these deep topics is also socially very challenging and you need some quite grown up people to discuss them (that late after the damage has been done) without awkward "but I spent all this time" moments and at least some personal animosity. But the grown ups think and discuss first typically, which also might be why you don't see them discuss those topics in PRs...

rwiggins 4y ago

One random thing I've been impressed with (that I know is public) is mutant testing: https://testing.googleblog.com/2021/04/mutation-testing.html https://testing.googleblog.com/2021/04/mutation-testing.html , https://research.google/pubs/pub46584/ https://research.google/pubs/pub46584/ To pull an example from the paper, if your line of code says if (a == b || b == 1) you might get a comment that says something like > "Changing this line to if (a != b || b == 1) > does not cause any test to fail." Page 4 lists other mutations, like replacing logical ANDs/ORs with just `true` or `false`, switching arithmetic plus to minus, etc. This isn't a larger system design context thing, though, just a testing one. Disclosure: I work at Google.

technion 4y ago
There's a pretty good Ruby gem I've used for this before: https://github.com/mbj/mutant https://github.com/mbj/mutant
YZF 4y ago
This is for test coverage? I get it technically but not clear to me if this adds value. Covering all the internal states/code paths in tests seems very hard and potentially of diminishing returns? Isn't time better invested in other things? I'm also guessing this ties into Google's tooling of being able to tell which line of code is hit by which tests and running those as part of the mutation test? EDIT: The blog has some discussion about the correlation between some of these mutation failures and real bugs. I remain a little suspicious to be honest. Also "I also looked into the developer behavior changes after using mutation testing on a project for longer periods of time, and discovered that projects that use mutation testing get more tests over time, as developers get exposed to more and more mutants. Not only do developers write more test cases, but those test cases are more effective in killing mutants: less and less mutants get reported over time. I noticed this from personal experience too: when writing unit tests, I would see where I cut some corners in the tests, and anticipated the mutant. Now I just add the missing test cases, rather than facing a mutant in my Code review, and I rarely see mutants these days, as I’ve learned to anticipate and preempt them." Now this one is expected, if you build automated tooling that comments on your code review you expect people to try and avoid that. However there's still the question of what's the quality improvement and what's the productivity impact.
- gravypod 4y ago
  Production impact: if you can replace: if (a == 10) With if (true) Then you're not testing the behavior of your application. If in production `a != 10` and you've never tested that it might be a problem. Mutation testing can also do stuff like removing entire statements, etc.
  - YZF 4y ago
    The question (well, one question) is why would we suspect the lack of a test indicates a bug? For most non-trivial software the possible state-space is enormous and we generally don't/can't test all of it. So "not testing the (full) behaviour of your application is the default for any test strategy", if we could we wouldn't have bugs... Last I checked most software (including Google's) has plenty of bugs. The next question would be let's say I spend my time writing the tests to resolve this (could be a lot of work) is that time better spent vs. other things I could be doing? (i.e. what's the ROI) Even ignoring that is there data to support that the quality of software where mutation testing was added improved measurably (e.g. less bugs files against the deployed product, better uptime, etc?) Is this method better than just looking at code coverage? Possibly none of the tests enter the if statement at all? EDIT: where I'm coming from is that it's not a given this is an improvement to the software development process. It seems like there was some experimentation around the validity of this method which is good but like a lot of software studies somewhat limited. It also seems there's a lot of heuristics based on user feedback, which is also good I guess, but presumably also somewhat biased. The related paper has a lot of details including: "Since the ultimate goal is not just to write tests for mutants, but to prevent real bugs, we investigated a dataset of high-priority bugs and analyzed mutants before and after the fix with an experimental rig of our mutation testing system. In 70% of cases, a bug is coupled with a mutant that, had it been reported during code review, could have prevented the introduction of that bug." Which should imply(???) that 70% of "high priority" bugs can be eliminated during the review process by using this sort of mutation testing. Seeing data to that effect would be cool (i.e. after the fact) and if it's real that'd be pretty incredible and we should all be doing that.
    - 4y ago

dilyevsky 4y ago

Fighting over style is resolved via eng-wide style guide and not having juniors approving each other’s code ;)

dodobirdlord 4y ago

One thing Google has going for it to make code review more efficient is the concept of a uniform per-language style guide. The style guides are absolute and comprehensive, meaning it’s not permitted to diverge from them, and equally importantly, that any style topics not covered by the style guide are not considered legitimate style issues to raise in code review. Formatting can be almost entirely automated with pre-submit checks that the code is formatted, and automatic formatting tools. So for your two examples, this practice of adopting a style guide prevents them from arising. The “tabs vs spaces” argument is simply settled, because the style guide has an answer. The “for loop vs for-each loop” is either addressed by the style guide, in which case there is an answer, or it’s not, in which case it is not a legitimate style complaint one way or the other, and the reviewer is making a mistake by bringing it up. https://google.github.io/styleguide/ https://google.github.io/styleguide/

elischleifer 4y ago
I would say any styleguide that is in prose form and not machine enforced is deficient. Modern linting and formatting tools are the best, most efficeiect means for enforcing style. No religious arguments are needed if the tooling decides what is correct. Shameless plug for https://trunk.io/products/check https://trunk.io/products/check - which will handle universal enforcement of all the tooling for all of the pieces of your tech stack.
0x20cowboy 4y ago
Yeah sorry I was just trying to pull something silly out of the air - we do mostly go and gofmt (or clangd or whatever). I was just trying to flowerly highlight that most “in depth code reviews” I’ve seen are very focused on surface level things. I’d dig a tool that focused more on structural changes.
- jeffreygoesto 4y ago
  Well. One could argue that if you have to discuss these very deep topics in a PR, you better had brought them up before coding that stuff wrong in the first place. Oh, that is not considered agile you say? Discussing these deep topics is also socially very challenging and you need some quite grown up people to discuss them (that late after the damage has been done) without awkward "but I spent all this time" moments and at least some personal animosity. But the grown ups think and discuss first typically, which also might be why you don't see them discuss those topics in PRs...