• ag0aep6g (24/24) Jul 19 2021 I filed issue 17635 [1] in 2017. A low-quality fix was merged later that...
• WebFreak001 (6/14) Jul 19 2021 in the last force push it seems the file was moved:
• ag0aep6g (15/21) Jul 19 2021 I'm not sure I know how to read these things, but it looks bad to me.

ag0aep6g <anonymous example.com> writes:

I filed issue 17635 [1] in 2017. A low-quality fix was merged later that 
year.

In 2018, Walter moved the test case for issue 17635 from /compilable/ to 
/fail_compilation/ [2]. I.e., he flipped it upside down. He provided no 
explanation. He did not update the issue in Bugzilla. I only found out 
about it now, because I'm working on a related issue and that test 
failed when I tried my fix.

The pull request was approved by Jacob Carlborg. He also had nothing to 
say about the flipping of the test case. But the branch was force-pushed 
eight times after the approval, so it's possible that the problematic 
change wasn't part of the version that Jacob reviewed. In which case, 
the approval should have been considered outdated.

If I were unreasonably amicable, I might say that Walter simply forgot 
to go back to Bugzilla and update the issue. But even then he should 
have mentioned in the pull request that it affects an older issue. And 
having dealt with Walter before, I'm not willing to find excuses 
anymore. I'd sooner believe that he snuck that change past Jacob with a 
force-push.

Changes to pre-existing tests must be done very carefully. When a pull 
request replaces a test with its exact opposite, that should be a big 
red flag that must be well justified.

Sadly, I can't be surprised that this happened. But I am pissed.


[1] https://issues.dlang.org/show_bug.cgi?id=17635
[2] https://github.com/dlang/dmd/pull/8048

WebFreak001 <d.forum webfreak.org> writes:

On Monday, 19 July 2021 at 10:54:47 UTC, ag0aep6g wrote:
 [...]

 If I were unreasonably amicable, I might say that Walter simply 
 forgot to go back to Bugzilla and update the issue. But even 
 then he should have mentioned in the pull request that it 
 affects an older issue. And having dealt with Walter before, 
 I'm not willing to find excuses anymore. I'd sooner believe 
 that he snuck that change past Jacob with a force-push.

 [...]

in the last force push it seems the file was moved:

https://github.com/dlang/dmd/compare/9e598d64c076d8b8c2f79f0214cddf3d55f75e11...ab5a18635adaa3e2271e0f4b569500a89ac74235

I haven't really thought about the content yet though, it might 
be that this could actually be a more correct behavior, which 
might be why Walter did this change.

ag0aep6g <anonymous example.com> writes:

On 19.07.21 16:55, WebFreak001 wrote:
 in the last force push it seems the file was moved:
 
 https://github.com/dlang/dmd/compare/9e598d64c076d8b8c2f79f0214cddf3d55f75e11...ab5a18635adaa3e2271e
f4b569500a89ac74235 

I'm not sure I know how to read these things, but it looks bad to me. 
AFAICT, this is the version that was reviewed:

https://github.com/dlang/dmd/commit/0b75c4567c012cc865d3e07ac50c08d59cca1800 
(2018-03-18T01:28:36Z)

fix17635.d was not touched in that version.

Jacob approved at 2018-03-18T11:36:52Z.

And this is the oldest version I can see that messes with fix17635.d:

https://github.com/dlang/dmd/commit/380ecff8fe281cff246b1b32c2e965a2691bd063 
(2018-03-19T21:40:04Z)

So it looks like Walter pushed non-trivial updates after the approval. 
And then he merged his own pull request based on the outdated approval.

 I haven't really thought about the content yet though, it might be that 
 this could actually be a more correct behavior, which might be why 
 Walter did this change.

It's not more correct. But even if it were, Walter would need to (1) 
explain that and (2) update the Bugzilla issue accordingly (mark invalid 
instead of fixed).