We need clearer rules on which pull requests require code reviews.
Weird to me that any pull request would not require a code review.
this post was submitted on 28 Jul 2023
35 points (97.3% liked)
Rust
5960 readers
2 users here now
Welcome to the Rust community! This is a place to discuss about the Rust programming language.
Wormhole
Credits
- The icon is a modified version of the official rust logo (changing the colors to a gradient and black background)
founded 1 year ago
MODERATORS
True, code for critical IT infrastructure should always be reviewed. But from what I understand, this is difficult because there is one full-time developer (paid by the Rust Foundation) and a small number of volunteers, who don't have the time to review all the employee's changes.
Easy solution, give review rights to a few volunteers. Pick from the regular contributors.
On GitHub, everybody has the ability to review pull requests, even you. But there still aren't enough volunteers who review PRs.
Sure, but you should always have a core contributor required to review code before it gets merged. That's a feature GitHub offers, and it should be used. Block all PRs unless there's at least one review from a trusted contributor, and consider requiring a second review from any source.
That doesn't solve the issue that there are too few contributors. Requiring a review doesn't ensure that someone reviews the code.
Requiring a review from a trusted contributor ensures that one of those trusted contributors reviews the code. The one main maintainer should add more people to that trusted circle, which will ensure that at least one of those will review all code that goes into the codebase.
If people see that code isn't being merged, someone will step up to request to be in that trusted circle.
Yeah. At my current and previous jobs, literally everything going into an actual product required a code review, and that's despite all the code being written by employees that you could generally trust. Even if my boss or literally the most experienced and trusted dev wrote a commit, it still needed a review.
It's feels weird submitting my own code without a review for side projects. So many bugs have been caught by reviewers that writing code that another person would use without it being reviewed feels just wrong.
The code had not been unit tested before
Because the smoke test procedure on our staging environment is currently a completely manual process without any automation.
Why do we have to keep learning to test and automate our tests as hard lessons?
Why do software engineering lectures not teach us about testing? If I were asked to teach software engineering (which TBH I shouldn't be qualified to do just yet) I'd start with testing.
I've always thought it weird that the intro CS course I took at my university didn't even mention unit testing. After being in the industry for several years, it's become obvious that the majority of what I do is just writing tests.
If you wanted to introduce every industry best practice in an intro course you'd never get to the actual programming.
It would be good to have a 1 credit course(one hour a week) where you learn industry best practices like version control, testing and stuff like that. But it definitely shouldn't be at the start.
Hard disagree. Cover less material if needed, but students should get into the habit of writing tests for everything they turn in. If I was a professor, I would reject any submitted code if it didn't have tests, for the same reason that math teachers reject work if students don't show their work.
There's a difference between tests and assertions. Students do test their code, however they don't write assertions, as I said because you want the cognitive load to be as low as possible so that they can master the basics. I'm fine with tests being provided to them, however they should be focusing on learning the constructs at the start.
In any field, the real life practice of a profession is something you learn working for an actual company, whether it's through an internship or an entry level job. Ideally there should be unions or syndicates setting these standards so that they're consistent across the field, just like with other knowledge based professions.
Universities are not corporate training programs, and they aren't supposed to be.
A huge part of computer science is proving correctness, complexity, etc. Almost all of my classes had an automated test suite that your code needed to pass to get full credit for the assignment. I think it's completely reasonable that you "show your work" by writing your own tests from the start.
If programming is just one or two classes of your program (e.g. you're doing IT or something), then I can insurance testing not being a part of it. But if you're going after a formal CS or CS-adjacent degree, you should be in the habit of proving the correctness of your code.
I'm totally fine with other industry norms being ignored, such as code style, documentation, and defensive programming, however, testing should absolutely be a regular part of any form of software development. I want every CS grad to always be thinking in terms of "how can I prove this" instead of just "how can I solve this." I don't think 100% code coverage should be expected, but students should prove the most important part of their solution.
I teachers were using automated tests instead of printf in their intro courses, it would be so much better. I don't think that introducing all the various kind of tests is usefull, but just showing the concept of automated tests instead of manual ones would be a huge step forward.
The thing is the way they motivate new students to learn programming is by having them write programs that do something. Making a test green isn't as motivating as visually seeing the output of your work, and test fixtures can be complex to set up depending on the language. I mean students don't learn how to factor their code into methods until later into such a course, they're learning if statements and for loops and basic programming constructs. Don't you think having to explain setting up test fixtures and dependency inversion is a bit too much for people at that level?
Honestly, that is weird. I wouldn't expect an intro course to go into a lot of depth on testing or even necessarily show how to use a test framework, but I'd expect them to at least have "printf style" unit tests.
But lol, yeah, tests usually take far longer to write than the actual change I made. A one line change might need a hundred lines of test code. And if you're testing something that doesn't already have a similar test that you can start off from, programming the test setup can sometimes take some time. Depends a lot on what your code does, but sometimes you have to setup a whole fake database and a hierarchy of resources with a mixture of real objects with stubs.
And then there's me, who almost never writes unit tests 😬
(With strong typing I can minimize explicit tests, and I like to iterate fast, but I guess it really depends on what you're developing, backend in production that is not allowed to fail, is probably something different than a game)
Unit tests shouldn't be testing types, even if your language isn't typed. It should be testing logic and behavior. If there's an if condition, it should be tested.
Yeah you're right, tests should test logic. But static typing certainly helps reducing a lot of tests, which would be necessary in different untyped languages. Also you can sometimes encode your logic in types. Typing also helps reducing logic issues. But as previously said, it depends on what you're doing. I'm prototyping/researching a lot, and tests often hinder progress for me. Maintaining a backend in production is a different story.
That is absolutely true as well. We're porting a codebase to TypeScript and we were able to eliminate a bunch of test cases that were essentially testing type-correctness (e.g. can't pass a boolean to a date processing library). But those were bad tests to begin with, because there was no good reason for those tests to exist to begin with (we were pretty exhaustive with the invalid type checking even when the intended types were obvious).
Strict typing helps eliminate useless tests. And Rust types go further than most languages, such as exhaustive match, types that can exclude zero, and the near-complete lack of a null value.
If you're never going to publish the code, I agree, tests aren't necessarily helpful. Then again, I find writing tests helps me understand my own code better, so I still do it when doing research tasks (e.g. we were testing the potential performance benefits of porting an expensive algorithm to Rust, so my tests helped me benchmark it), though my tests are a lot less exhaustive and tend to be more happy path integration tests instead of proper unit tests.
I find writing tests helps me understand my own code better, so I still do it when doing research tasks
Hmm interesting, I try to optimize readability of the actual code itself, so that when I read it again after some time, that I quickly get what this is about, if there's a edge-case or something I thought about while coding, I'll just add a TODO comment or something like that. I feel like reading tests is a "waste of time" for me most of the time (hard take I know ^^).
But all this obviously only applies for researching and fluid code (code that likely will be refactored/rewritten soon), when it's solid code targeting production etc. I'll add unit tests if friction/hassle is low, and integration/E2E tests for the overall thing. But as I said, I'm mostly in fluid/fast moving codebases that are more difficult to test (e.g. because it does gpu rendering or something like that).
When I jump into a new codebase, my first instinct is to look over the examples and the unit tests to get a feel for how things are intended to work.
When prototyping, I generally write a few "unit" tests as a form of example code to show basic usage. For example, if I'm writing a compiler for a new toy language, I'll write some unit tests for each basic feature of the language. If I'm writing networking code (e.g. a game server), I'll write some client and server tests that demonstrate valid and invalid packets. These generally fall somewhere between unit and integration tests, but I do them in a unit test style. When the project stabilizes, I'll go through and rewrite those tests to be narrower in scope, broader in line coverage, and simpler, and keep a few as examples (maybe extract to the readme or something).
That's my workflow, and I like knowing that at least part of it is tested. When I mess with stuff, I have a formal step to change the tests as a form of documenting the change I made, and I'll usually leave extensive comments on the test to describe the relevance.
Code readability counts, but I don't think it's enough. The codebase I work on day to day is quite readable, but it's very complex since there are hundreds of thousands of lines of code across over a dozen microservices, and there's a lot of complexity at the boundaries. When I joined the project, I read through a lot of the tests, which was way more helpful to me than reading the code directly. The code describes "how," but it doesn't explain "what" or "why." Tests get into "what" extensively, and "why" can be understood by the types of tests developers choose to write.
For example, if I’m writing a compiler for a new toy language
Ok, thinking about it (since I wrote a toy language not so long ago), this is probably a perfect example where unit tests make sense almost everywhere (even for prototyping, say parser).
I think it definitely depends what you're doing, writing unit tests for prototype graphics (engine) code is no fun (and I see no real benefit).
Code readability counts, but I don’t think it’s enough.
I think it depends, For general architecture, E2E or integration tests definitely make sense, for finer-grained code, I think documentation (Rust doc) of the functions in question should be enough to understand what they do (including some examples how to use them, could be tests, often examples (similar as in std rust) in the rust doc are enough IMHO, and otherwise the code itself is the documentation (being able to read code fast is a valuable skill I guess). That obviously doesn't apply for everything (think about highly theoretical computer science or math code), but yeah it depends...
Yeah, I wouldn't bother for graphics code either. For that, I want compilable examples, and that's about it.
I do a lot of math and parsers, and that lends itself very well to unit tests.
Strong typing doesn't prevent the need for tests. It can certainly catch some issues (and I don't like dynamically typed languages as a result), but there's no replacement for unit testing. So much refactoring is only safe because of rigorous test coverage. I can't begin to tell you how many times a "safe" refactoring actually broke something and it was only thanks to unit tests that I found it.
If code is doing anything non-trivial, tests are pretty vital for ensuring it works as intended (and for ensuring you don't write too much code before you realize something doesn't work). Sure, you can manually test, but often manual testing can have a hard time testing edge cases. And manual testing won't help you prevent regressions, which is usually the biggest reason to write unit tests. If you have a big, complicated system worked on by more than one person, tests can be critical for ensuring other people (who often have no idea how your code works) don't break your test. Plus your own future changes.
view more: next ›