/test

/test

@rgo3 @nathanjsweet @pippolo84 @julianwiedmann @viktor-kurchenko @ldelossa How did this slip through in #38452 ? I'd like to dig a bit more to understand whether there is a systematic problem here in the way that we approach development and reviews. Bear with me if you think these concerns are overblown.

I'm not so concerned about the content of the change here itself, admittedly it's in test code which is less critical. What concerns me more is that at no point during development or review did someone look at this line of code and make a judgement value about whether it makes sense. Are we all consistent in the belief that when you review a change, you need to put eyes on every line of code for your codeowners and decide if you understand how that code fits into the PR?

I would expect at least two people to look at this line of code before it goes into the tree: the contributor and the reviewer for the code owner area. I can understand if there are mistakes, but that's partly why we have a system that involves at least two people looking at the code.

I'd like to dig a bit more to understand whether there is a systematic problem here in the way that we approach development and reviews.

I remember reviewing that test too, but probably because I was focused on the coverage of the IPv6 policy map operations and the consistency with the IPv4 counterpart I must have missed the offending line. My mistake, sorry for that.

I won't say there is a systematic problem, though, I remember catching many similar leftovers in many other PRs. I wonder if there is something we can do to prevent this from happening again with a linting of some sort, but a quick search in the codebase reveal some legitimate usages of fmt.Println in other tests. A further reason to be extra careful in future reviews.

Are we all consistent in the belief that when you review a change, you need to put eyes on every line of code for your codeowners and decide if you understand how that code fits into the PR?

Sure, this has always been clear. Again, will put extra care to avoid similar mistakes in the future.

@rgo3 @nathanjsweet @pippolo84 @julianwiedmann @viktor-kurchenko @ldelossa How did this slip through in #38452 ? I'd like to dig a bit more to understand whether there is a systematic problem here in the way that we approach development and reviews. Bear with me if you think these concerns are overblown.

I'm not so concerned about the content of the change here itself, admittedly it's in test code which is less critical. What concerns me more is that at no point during development or review did someone look at this line of code and make a judgement value about whether it makes sense. Are we all consistent in the belief that when you review a change, you need to put eyes on every line of code for your codeowners and decide if you understand how that code fits into the PR?

I would expect at least two people to look at this line of code before it goes into the tree: the contributor and the reviewer for the code owner area. I can understand if there are mistakes, but that's partly why we have a system that involves at least two people looking at the code.

No excuse, I'm super sorry for that. Probably I didn't pay enough attention to the test.
I'll put extra care into avoiding it in the future!

I’m not sure if this points to a broader systemic issue, but my sense is that this kind of oversight can easily happen in larger PRs, especially when a significant portion of the diff comes from boilerplate or already-reviewed code (e.g., changes in indentation). Particularly in cases like this, where tests are extended, and the review potentially focuses largely on the comparison of the tests functionality (v4 vs. v6), the chances of overlooking a typo or a unwanted print statement in order to maintain a certain velocity will never be 0.

So if we want to avoid such slip ups at all cost, I agree with @pippolo84 that we should have a linter workflow that points out any new occurrences of print statements.