Page MenuHomePhabricator

tests: update test-copies-chain-merge.t to not use empty files
ClosedPublic

Authored by martinvonz on Thu, Oct 8, 12:17 PM.

Details

Summary

Merging empty files is not very interesting or realistic.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Thu, Oct 8, 12:17 PM
martinvonz updated this revision to Diff 23122.Fri, Oct 9, 2:28 AM
pulkit accepted this revision.Fri, Oct 9, 5:03 AM
This revision is now accepted and ready to land.Fri, Oct 9, 5:03 AM
tests/test-copies-chain-merge.t
780–788

(rewritting this since phab apparently lost the initial writting)

This is unexpected and unexplained output changes. So I strongly suspect that this patch is silently changing the test coverage. I would prefer that we add explicit test case for file being actually merged instead of changing the existing coverage.

This unexpected test change is, in my opinion, a strong enough reason to drop this patch and resume review on a new patch.

In addition, this patch conflicts with a larger series I submitted that fixes two important bugs: (see D9195 and descendants). So I would much prefer to have this patch dropped for now and a more robust successors build above my bugfix series.

martinvonz added inline comments.Tue, Oct 13, 12:01 PM
tests/test-copies-chain-merge.t
780–788

This is unexpected and unexplained output changes.

Is it *more* unexplained now than before? The comment on line 745 says "The current code arbitrarily pick one side". That still seems valid.

So I strongly suspect that this patch is silently changing the test coverage. I would prefer that we add explicit test case for file being actually merged instead of changing the existing coverage.
This unexpected test change is, in my opinion, a strong enough reason to drop this patch and resume review on a new patch.

The only thing that changes is the filelog case. That's not too surprising given that the filelog changes with this patch, and it's not something you've changed recently, so I'm not worried that you've broken it. Also, even if it's testing a slightly different case, the filelog-centric copy tracing has been there forever and has its own testing, so I'm also not concerned that we've lost a very important test case for it.

In addition, this patch conflicts with a larger series I submitted that fixes two important bugs: (see D9195 and descendants). So I would much prefer to have this patch dropped for now and a more robust successors build above my bugfix series.

D9195 was sent after this patch, so I have little sympathy for that reason.

marmoute added inline comments.Tue, Oct 13, 3:12 PM
tests/test-copies-chain-merge.t
780–788

Is it *more* unexplained now than before? The comment on line 745 says "The current code arbitrarily pick one side". That still seems valid.

My general expectation is for change affecting tests to explain why, which was missing here. You explanation of "this is an arbitrary pick seems fine.

However the main problem remains, why are we doing this change at all?

  • Either we expect that having content in the file will have absolutely no effect or anything. Then making this change is purely gratuitous.
  • Or we do expect this to matters, in which case we should introduce new test cases instead of removing the olds one in favor of the new ones.

So, as I see it, we currently have either a problematic change in the current test coverage or a gratuitous changes that get in a the way of more important, actual, semantic fixes. These actual semantic fixes that have been made as part of an active, long running series of work on this topic. The creation of this patch not being coordinated with the author of that active series of work. does not help.