[dead]
There is an AI code review bubble
241–250 of 265 posts
Re: There is an AI code review bubble
#242I've tried Greptile and it's pretty much pure noise. I ran it for 3 PRs and then gave up. Here are three examples of things it wasted my time on in those 3 PRs: * Suggested to silence exception instead of crash and burn for "style" (the potential exception was handled earlier in code but it did not manage to catch that context). When I commented that silencing the exception could lead to uncaught bugs it replies "You…
[dead]
Re: There is an AI code review bubble
#243Earlier quoted context omitted.
I know how I would do it in python. This is built into the stdlibs testing library, with mocks. Maybe dependency injection and function pointers for the copy if function. Then you can check the call counts in your tests. But idk the cpp eco system and what's available there to do it.
The python code would be def some_call(): return [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] def print_evens(nums): for n in filter(lambda n: n % 2 == 0, nums): print(n) def func(): filtered = list(filter(lambda n: n % 2 == 0, some_call())) print_evens(filtered) if __name__ == "__main__": func() How would you write a failing test that prevents the list from some_call() from having the same filter applied to it twice?
You would use something like this
https://docs.python.org/3/library/unittest.mock.html#unittes...
Then you would make the mock filter with patch and test the `func` function
psuedo python code would be
@patch(builtins.filter)
def test_func_filter_calls(mock_filter):
mock_filter.return_value = [2,4,6]
func()
mock_filter.assert_called_once_with([1, 2, 3, 4, 5, 6, 7, 8, 9, 10])Re: There is an AI code review bubble
#244Earlier quoted context omitted.
That sounds great. Was that proprietary tooling? I'd be interested in some such thing.
Yeah, it's called git: make your own branch from the PR branch, commit and push the nitpick change, tell the author, and they can cherry-pick it if they approve. Gitlab has this functionality right in the web UI. Reviewers can suggest changes, and if the PR author approves, a commit is created with the suggested change. One issue with this flow it that's it doesn't run any tests on the change before it's actually in…
I think you misunderstood the tooling I was asking about. This is what was mentioned:
> at my last job code review was done directly in your editor (with tooling to show you diffs as well).
That's not covered by git itself. And it's not covered by Gitlab, GitHub, or any other web-based forge.
> Alternatively you actually, you know, _collaborate_ with the PR author, work it out, run tests locally and/or on another pushed branch, and someone then pushes a change directly to the PR.
Of course you should collaborate with the author. This tooling is a specific means to do that. You yourself are of course free to not like such tooling for whatever reason.
> The complaints about nitpicks slowing things down too much or breaking things sound like solo-hero devs who assume their god-like PRs should be effectively auto-approved because how could their code even contain problems... No wonder they love working with "Dr Flattery the Always Wrong Bot".
Did you maybe respond to the wrong person? I'm not sure how that relates to my comment at all.
Re: There is an AI code review bubble
#245Earlier quoted context omitted.
The python code would be def some_call(): return [1, 2, 3, 4, 5, 6, 7, 8, 9, 10] def print_evens(nums): for n in filter(lambda n: n % 2 == 0, nums): print(n) def func(): filtered = list(filter(lambda n: n % 2 == 0, some_call())) print_evens(filtered) if __name__ == "__main__": func() How would you write a failing test that prevents the list from some_call() from having the same filter applied to it twice?
You would use something like this https://docs.python.org/3/library/unittest.mock.html#unittes... Then you would make the mock filter with patch and test the `func` function psuedo python code would be @patch(builtins.filter) def test_func_filter_calls(mock_filter): mock_filter.return_value = [2,4,6] func() mock_filter.assert_called_once_with([1, 2, 3, 4, 5, 6, 7, 8, 9, 10])
That wouldn't fail though. It was called only once with [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]. The second time it was called with [2, 4, 6, 8, 10].
Likewise, if some_call returned [2, 4, 6, 8, 10] instead, it should only be called with [2, 4, 6, 8, 10] once then.
However, the purpose of this test then becomes questionable. Why are you testing implementation details rather than observable? Is there anything that you could observe that depended on the filter being called once or twice with the same filter function?
Re: There is an AI code review bubble
#246Earlier quoted context omitted.
You would use something like this https://docs.python.org/3/library/unittest.mock.html#unittes... Then you would make the mock filter with patch and test the `func` function psuedo python code would be @patch(builtins.filter) def test_func_filter_calls(mock_filter): mock_filter.return_value = [2,4,6] func() mock_filter.assert_called_once_with([1, 2, 3, 4, 5, 6, 7, 8, 9, 10])
That wouldn't fail though. It was called only once with [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]. The second time it was called with [2, 4, 6, 8, 10]. Likewise, if some_call returned [2, 4, 6, 8, 10] instead, it should only be called with [2, 4, 6, 8, 10] once then. However, the purpose of this test then becomes questionable. Why are you testing implementation details rather than observable? Is there anything that you could o…
Did you try it? If it doesn't work there's also called once if you scroll up on the doc
And as far as whether it's a good idea or not, I generally wouldn't, but was saying when it is important then you do have these tools available,llms aren't the first thing to check for these mistakes. It's up to the engineer to choose between trade offs for your scenario.
Re: There is an AI code review bubble
#247We built an internal code review tool at the day job and are getting pretty good results with it (CLI tool). Here's a summary of the top-level ideas behind it. Hope it's helpful! Core Philosophy - "Advisor, not gatekeeper" - Every issue includes a "Could be wrong if..." caveat because context matters and AI can't see everything. Developers make the final call. (Just this idea makes it less annoying and stops devs goi…
If you need the AI to indicate "could be wrong" on everything it writes to prevent your devs from blindly following everything it says, you're doing it so wrong. That should be the default mindset. Of course it could be wrong.
Re: There is an AI code review bubble
#248Earlier quoted context omitted.
That wouldn't fail though. It was called only once with [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]. The second time it was called with [2, 4, 6, 8, 10]. Likewise, if some_call returned [2, 4, 6, 8, 10] instead, it should only be called with [2, 4, 6, 8, 10] once then. However, the purpose of this test then becomes questionable. Why are you testing implementation details rather than observable? Is there anything that you could o…
Did you try it? If it doesn't work there's also called once if you scroll up on the doc And as far as whether it's a good idea or not, I generally wouldn't, but was saying when it is important then you do have these tools available,llms aren't the first thing to check for these mistakes. It's up to the engineer to choose between trade offs for your scenario.
Yes. The test passes. https://imgur.com/a/4qlTKlc
To try to monkey patch this in, you would need to also assert that it wasn't called with [2, 4, 6, 8, 10].
At which point, I would again ask "why are you testing that it _wasn't_ called with a given set of values?"
The comment at the root of this is "Unit tests catch that kind of stuff".
... But unit tests aren't for testing internals of implementation but rather observable aspects of a function.
Consider if the code was written so that it was
def print_evens(nums):
for n in nums:
if n % 2 == 0:
print(n)
This isn't something that unit tests can (or should) identify. It would come out in a code review that there is redundant functionality in func and print_evens.
Using ChatGPT or another tool to assist in doing code reviews can be helpful (my original premise).
https://chatgpt.com/share/697a64a6-33c0-8011-a0f8-ca4fec74ab...
ChatGPT properly identifies the duplicated functionality (even though the code is using different idioms for doing the filtering for even numbers).
Re: There is an AI code review bubble
#249Earlier quoted context omitted.
Did you try it? If it doesn't work there's also called once if you scroll up on the doc And as far as whether it's a good idea or not, I generally wouldn't, but was saying when it is important then you do have these tools available,llms aren't the first thing to check for these mistakes. It's up to the engineer to choose between trade offs for your scenario.
Yes. The test passes. https://imgur.com/a/4qlTKlc To try to monkey patch this in, you would need to also assert that it wasn't called with [2, 4, 6, 8, 10]. At which point, I would again ask "why are you testing that it _wasn't_ called with a given set of values?" The comment at the root of this is "Unit tests catch that kind of stuff". ... But unit tests aren't for testing internals of implementation but rather obse…
The test pass because your patching print_even so the 2nd filter is never called.
Which I guess, idk maybe think through the testing more and your code more before jumping to conclusions about how things are?
Testing is one tool you have, and it can test the internal like this. Obviously there's a use for it if its in the Python stdlib
This is in mockito
https://stackoverflow.com/questions/39452438/mockito-how-to-...
this is in google testing library for cpp
https://google.github.io/googletest/gmock_for_dummies.html
> Specify your expectations on them (How many times will a method be called? With what arguments? What should it do? etc.).
If you never heard of this, I guess you learned something new? Im not a tutor though. I would read the docs more and experiment. Maybe chatgpt can help you with how tests can be written.
Re: There is an AI code review bubble
#250Hot take: Code review is an anti-pattern. We spend a ton of time looking at the code and blocking merges, and the end result is still full of bugs. AI code review only provides a minor improvement. The only reason we do code review at all is humans don't trust that the code works. Know another way to tell if code works? Running it. If our code is so utterly inconceivable that we can't make tests that can accurately a…
This is how you end up with O(n^2) algorithms that 'work' until you run it with production-size data.
If the context your code runs in is small enough to be in a test, you're probably not working on anything serious anyway.