Page MenuHomePhabricator

test-copies: reinstall initial identical (empty) files for chained copied
ClosedPublic

Authored by marmoute on Dec 14 2020, 6:17 AM.

Details

Summary

This effectively back out changeset deeb215be337. Changeset deeb215be33 does not
really include a justification for its change and make mes uncomfortable. I have
been thinking about it and they are two options:

  • either having empty/full files does not make a difference, and deeb215be337 is a gratuitous changes.
  • either having empty/full files do make a difference and deeb215be33 silently change the test coverage. In such situation if we want the "not empty" case to be tested, we should add new cases to cover them

In practice, we know that the "file content did not change, but merge still need
to create a new filenode" case exists (for example if merging result in similar
content but both parent of the file need to be recorded), and that such case are
easy to miss/mess-up in the tests. Having all the file using the same (empty)
content was done on purpose to increase the coverage of such corner case.

As a result I am reinstalling the previous test situation. To
increase the coverage of some case involving content-merge in
test-copies-chain-merge.t, we will add a new, dedicated, cases later in this
series, once various cleanup and test improvement have been set in place.

This changeset starts with reinstalling the previous situation as (1) it is more
fragile, so I am more confided getting it back in the initial situation, (2) I
have specific test further down the line that are base on these one.

The next changeset will slightly alter the test to use non-empty files for these
tests (with identical content). It should help to make the initial intent "merge file with identical
content" clearer. I am still using a two steps (backout, then change content)
approach to facilitate careful validation of the output change.

Doing so has a large impact on the output of the "copy info in changeset extra" variant
added in 5e72827dae1e (2 changesets after deeb215be33). It seems to highlight
various breakage when merge without content change are involved, this is a good
example of why we want to explicitly test theses cases. Because the different
-do- matters a lot.

Fixing the "copy info in changeset extra" is not a priority here. Because (1)
this changeset does not break anything, it only highlight that they were always
broken. (2) the only people using "copy info in changeset extra" do not have
merge.

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

marmoute created this revision.Dec 14 2020, 6:17 AM
baymax updated this revision to Diff 24251.Dec 14 2020, 9:15 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

martinvonz requested changes to this revision.Dec 15 2020, 11:37 AM
martinvonz added a subscriber: martinvonz.

I don't see how this improves testing. I won't queue it and I won't give it an accept stamp. Other reviewers can of course disagree with me.

This revision now requires changes to proceed.Dec 15 2020, 11:37 AM
baymax edited the summary of this revision. (Show Details)Dec 17 2020, 3:53 PM
baymax updated this revision to Diff 24364.

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 24386.Dec 18 2020, 8:40 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

Alphare accepted this revision.Dec 18 2020, 12:06 PM
Alphare added a subscriber: Alphare.

It looks like @marmoute has added additional justification for the change. To me, this looks like a decent strategy: prioritize the cases you know to fail, and then slowly go over the rest.
There are clearly corner-cases that we're hitting when using empty files, they're worth testing, as long as the tests for non-empty files come along in a future patch to not lose coverage one way or the other.
I have good insider information that the future patches will come in time, hehe.

martinvonz requested changes to this revision.Dec 18 2020, 1:26 PM

It looks like @marmoute has added additional justification for the change. To me, this looks like a decent strategy: prioritize the cases you know to fail, and then slowly go over the rest.
There are clearly corner-cases that we're hitting when using empty files, they're worth testing, as long as the tests for non-empty files come along in a future patch to not lose coverage one way or the other.
I have good insider information that the future patches will come in time, hehe.

Can we at least move it out of this series? And please do it thoroughly and thoughtfully across the entire test suite: go through the test suite and see which tests would work just as well with empty files and update all of them, and of course add duplicate versions with non-empty tests where it might matter (as it does for copy tracing without data in the filelog).

This revision now requires changes to proceed.Dec 18 2020, 1:26 PM

It looks like @marmoute has added additional justification for the change. To me, this looks like a decent strategy: prioritize the cases you know to fail, and then slowly go over the rest.
There are clearly corner-cases that we're hitting when using empty files, they're worth testing, as long as the tests for non-empty files come along in a future patch to not lose coverage one way or the other.
I have good insider information that the future patches will come in time, hehe.

Can we at least move it out of this series?

This changeset is part of this series because I am adding more test down the line. And I want to restore the intended test coverage of test-copies-chain-merge.t before doing so, so that the new test have the right coverage too.

And please do it thoroughly and thoughtfully across the entire test suite: go through the test suite and see which tests would work just as well with empty files and update all of them, and of course add duplicate versions with non-empty tests where it might matter (as it does for copy tracing without data in the filelog).

As explained in this patch, I am planning for a more thoughtful review and consolidation of the test coverage of copy tracing. However bandwidth is limited and I currently focus on covering and fixing some specific cases. As explained in the first part of this reply, I am backing out a regression in test coverage as part of this focus.

baymax updated this revision to Diff 24447.Dec 21 2020, 2:43 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

baymax updated this revision to Diff 24573.Jan 5 2021, 3:38 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

I had a meeting earlier with @marmoute today and we discussed this. After my discussion and reviewing this patch, I understood:

  1. the motive is to test merging files that have same file content on both sides. So instead of empty files, we probably can have a version of this patch that writes out the same content.
  2. Having the same content is breaking tests for changeset case. Refer changes around line-354 on both sides.

The 2) part makes me think that we should have tests for merging files with the same content. I don't have a strong opinion on how that should be done[1]. @durin42 @mharbison72 thoughts on that?

@marmoute @martinvonz correct me if I got something wrong.

[1]: IIRC, I pushed Martin's original patch which added content to files and though Pierre-Yves later commented against it, I didn't understand the hidden benefit that empty files were bringing i.e. merging files with the same content

I had a meeting earlier with @marmoute today and we discussed this. After my discussion and reviewing this patch, I understood:

  1. the motive is to test merging files that have same file content on both sides. So instead of empty files, we probably can have a version of this patch that writes out the same content.

We can probably have a version of these tests using some explicite content that is indentical on purpose. That will probably requires to adjustement during some merge to make sure we keep the same content 2 content situation. I can follow up with such patch if you want. I would rather keep this one a backout of the on regressing the coverage to avoid mixing different changes.

  1. Having the same content is breaking tests for changeset case. Refer changes around line-354 on both sides.

The 2) part makes me think that we should have tests for merging files with the same content. I don't have a strong opinion on how that should be done[1]. @durin42 @mharbison72 thoughts on that?
@marmoute @martinvonz correct me if I got something wrong.

I think you got things right.

[1]: IIRC, I pushed Martin's original patch which added content to files and though Pierre-Yves later commented against it, I didn't understand the hidden benefit that empty files were bringing i.e. merging files with the same content

durin42 requested changes to this revision.Jan 11 2021, 2:25 PM

I guess I don't really understand the position of "we should back this out" particularly. Given that it's a test-only change, could we just roll-forward with the new test discussed (that is, a variant of this test with the content the same for all the files)?

AIUI that's the desired end state: a version of this test with contents-same and contents-different files, and the empty files is merely a path to having the contents-same variant. Right? I'd rather not bikeshed about the particular path function to get there, but I'm also not really interested in landing _this specific patch_ because it doesn't improve things - it's just picking the "files all match" variant rather than the "files differ" variant, but in a way that (I think?) most of us agree is worse (empty files).

So please either update this patch to introduce a non-empty matching-file version of the test, or let's drop it for now.

This revision now requires changes to proceed.Jan 11 2021, 2:25 PM
marmoute planned changes to this revision.Mon, Feb 15, 10:26 AM

I finally have to get back the copy tracing test. So here is quick heads up on my current plan:

  1. backout the content change (this changeset) to get back to a previously validated state
  2. change the content of the "same-content" file while carefully making sure the test preserver their current property
  3. introduce new explicit test for the "merge different content" case.

They might be some wider rework of the tests at some point in the process (probably between 2 and 3, or after 3).

Since the compatibility changeset case runs code knowly misbehave with merge, and that they am I not aware of any plan to fix that (because the sole user-group of theses options does not use Merge), I expect them to be cumbersome to support while we refactor the tests. They often requires conditional matching of output (since they give wrong results) and their output is not really worth checking-and-preserving (since we know them to be wrong). Depending on how combersum this turn out, I might drop these cases sometwhere in the process. (Unless of course if someone can make a compelling argument of why they should be tested there).

Keep in mind that no plan survive first contact with reality. I'll ping this Diff again when I am done reworking the tests.

I finally have to get back the copy tracing test. So here is quick heads up on my current plan:

  1. backout the content change (this changeset) to get back to a previously validated state
  2. change the content of the "same-content" file while carefully making sure the test preserver their current property
  3. introduce new explicit test for the "merge different content" case.

They might be some wider rework of the tests at some point in the process (probably between 2 and 3, or after 3).
Since the compatibility changeset case runs code knowly misbehave with merge, and that they am I not aware of any plan to fix that (because the sole user-group of theses options does not use Merge), I expect them to be cumbersome to support while we refactor the tests. They often requires conditional matching of output (since they give wrong results) and their output is not really worth checking-and-preserving (since we know them to be wrong). Depending on how combersum this turn out, I might drop these cases sometwhere in the process. (Unless of course if someone can make a compelling argument of why they should be tested there).
Keep in mind that no plan survive first contact with reality. I'll ping this Diff again when I am done reworking the tests.

I have been spend a significant part of my week on this. step (1) and (2) went fine without much pain. I am now working on slowly adding variant of the test that involve merges that involves identical file. They are mostly two differents variants that seems relevant:

  1. case where the merge still involve identical files, but then introduce extra change before committing
  1. case where merge involves files with different content.

Progress are steady, but the whole mass of work is significant since validating out the output change is tedious and slow.

The "good news" is that some of the new tests already yield some bugs in the current code.

  1. case where the merge still involve identical files, but then introduce extra change before committing

These one reveal various issue in the merge-code (affecting filelog based copy tracing) and the associated sidedata recording (including mismatch between commit-time and upgrade-time data). This will requires more work.

In the mean time the test adding coverage for merge involving different content have been added and does not reveal any new bug. I'll send them shortly once I'll be done rebasing and retesting the multiple dozen of patch that depends on this.

marmoute retitled this revision from copies: reinstall initial empty files for chained copied to test-copies: reinstall initial identical (empty) files for chained copied.Mon, Feb 22, 9:33 AM
marmoute edited the summary of this revision. (Show Details)
marmoute updated this revision to Diff 25707.

This series is ready to be re-reviewed.

(for those curious, the piece about adding and merging files with different content are in D10053 to D10059)

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.