We need clearer rules on which pull requests require code reviews.
Weird to me that any pull request would not require a code review.
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.
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.
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)
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.
Funny how you got successfully distracted by the procedural failure dance, where the obvious, as expected, got zero mentions. Giving software engineering lectures seems to be right up your alley.
If I was the author of that commit, or any
crates.iodeveloper, I would have wanted to be called out for not constructing URLs correctly. That’s the obvious first fault here. Not even hinting at that would have felt so cringe.I can’t tell if your comment is intentionally sarcastic but it sure sounds like you’re saying “just don’t write buggy code in the first place!”
It’s about not ignoring the clear underlying cause of the bug that is screaming at everyone who reads the bug description.
Include something along the lines of “We will use the URL crate and utilize its API to avoid trivial URL construction errors like this one in the future”, and I may take your postmortem seriously.
A flawless developer does not exist, and at no point did I fault any developer directly for their development work. But that doesn’t mean we should ignore something that is/was clearly and inherently wrong with the code. You would think this is all stating the obvious.
So it’s not "just don’t write buggy code in the first place!”. It’s “this code could clearly have been written in a way that would have prevented this bug from ever taking place”.
And yes, good code matters. A good language matters. A good type system matters. A good use of a good language with its type system, patterns, abstractions, ecosystem, and all it got to offer matters. This is Rust afterall. If those things don’t matter, then we might as well let the code be written in Python or JS, and fully recommit to the church of TDD.
That basically is the same as saying “next time we will write correct code” in your postmortem, which I don’t think is very useful. It’s much more useful to say “our code is not structured in a way that makes testing easy” and “our smoke tests should cover the thing that broke.” That gives you something actionable to work on that will actually prevent this from happening in the future. Otherwise, you’ll end up writing essentially the same postmortem over and over again, each time saying “we will write correct code.”
False dichotomy much!
See this postmortem from Cloudflare as an example.
Under “What went wrong”, point 1 and 3:
1. An engineer wrote a regular expression that could easily backtrack enormously.
3. The regular expression engine being used didn’t have complexity guarantees.
And on what needed to done, point 4
4. Switching to either the re2 or Rust regex engine which both have run-time guarantees.
See! Plenty of procedural talk in that postmortem. Plenty of corporate talk too. But you have to mention that a bad backtracking regex was used. And you have to mention that using regexes with no complexity guarantees was glaringly wrong. To not have done so would have been silly. To not even come close to mentioning those things beyond the specific error in that specific regex, and you wouldn’t have been taken seriously.
A good language matters. A good type system matters. A good use of a good language with its type system, patterns, abstractions, ecosystem, and all it got to offer matters.
Eh research shows otherwise. Rust eliminates defects for a very particular set of problems, but when it comes to logical correctness it isn’t better or worse than other languages. If those problems are prominent in your domain(such as you have to write a ton of concurrent code), Rust makes sense. Otherwise being well rested will have a bigger impact on the quality of your code than the best type system in the world.
In terms of dev practices, the only practice demonstrated to have a consistent positive impact on code quality is code reviews. Testing as well, but whether it’s TDD or other kinds of testing doesn’t really matter.
research shows otherwise
Can you share that research?
It’s not that there is evidence that it doesn’t matter, but there is no evidence showing that it does.
Here is an alternative Piped link(s): https://piped.video/WELBnE33dpY
Piped is a privacy-respecting open-source alternative frontend to YouTube.
I’m open-source, check me out at GitHub.
Eh research shows otherwise. Rust eliminates defects for a very particular set of problems, but when it comes to logical correctness it isn’t better or worse than other languages.
Can you concede, at least to yourself, that you made ^ this ^ up?
By the way, what you claimed “research shows” is so ridiculous that it’s hilarious that you wrote it while being serious.
Hell, I cheekily mentioned Python and JS in particular because the former introduced type hints and the latter triggered creating TS as a saner shield.
Btw, that wrongly-constructed URL wasn’t even an external one. We literally have web frameworks that make sure non-external URLs with invalid paths are impossible to construct. In other words, attempting to construct a wrong one would be a compile error.
Honestly
url.joinlooks like a cluster-fuck of landmines to me: https://github.com/servo/rust-url/issues/333I’d probably have just stuck with strings as well.
I’d probably have just stuck with strings as well.
And this argument works as long as nothing wrong happens. Well, something wrong happened ;)
Smashing strings together is how this bug happened.
Constructing URLs reliably should have been the obvious first takeaway, was my point, instead of pretending the issue is not there. If
Url::join()is somehow too confusing for some, then there are other ways to do it with simpler API, no problem.
I would have wanted to be called out for not constructing URLs correctly.
You might have overlooked that we do not start out as experts. It is simply impossible. There is no way to guarantee that we know how to do things 100% correctly before writing correct code. Even if we were experts, we’re still humans, we’ll screw something up. This is just one of the reasons why we write proper tests and automate them.
