Page MenuHomePhabricator

copies: add a new test dedicated to testing chain of changeset with merge
ClosedPublic

Authored by marmoute on Feb 5 2020, 9:37 AM.

Details

Summary

The copies test we currently have usually focus on simple case that do not dive
too much into longer chains involving merges. This new test file focus on
extensive testing of these case to validate their behavior and make sure the
various copies algorithm have the same behavior.

And… actually these test are currently broken for the changeset centric
algorithm since 99ebde4fec99, but it went undetected because these case were not
tested.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

marmoute created this revision.Feb 5 2020, 9:37 AM
martinvonz requested changes to this revision.Feb 5 2020, 11:32 AM
martinvonz added a subscriber: martinvonz.

Missing (proper) commit message. I assume the title line would be something like "tests: add tests for copies across a chain of merges", but I'm not sure what the body would be.

This revision now requires changes to proceed.Feb 5 2020, 11:32 AM

oops, this one got out by mistake.

marmoute retitled this revision from tests/test-copies-chain-merge.t to copies: add a new test dedicated to testing chain of changeset with merge.Feb 6 2020, 5:46 AM
marmoute edited the summary of this revision. (Show Details)
marmoute updated this revision to Diff 19931.

oops, this one got out by mistake.

But now it's ready for review (seems ready to me)? Thanks for adding these tests.

Yeah, not it is. I added extra comment and output to review and clarify things.

FYI, coming next on this topic is fixing some inconsistency and then fixing then changeset centric behavior. (but do not hold your breath, nodemap has an higher priority)

pulkit accepted this revision.Feb 7 2020, 3:25 PM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.

Sorry that I missed the review of this patch earlier. I hope you can still address my comments and send any resulting changes as follow-up patches in the same series as D8235.

tests/test-copies-chain-merge.t
2

Why doesn't this one have a #testcases line like test-copies.t does? It seems useful to check that how these cases work when copies are stored in changesets or sidedata too, right?

8

This sounds like it's talking about that #testcases thing, right? You planned to add it earlier and then didn't? Why not?

13

doesn't seem used

61

I'm confused by this. It doesn't seem to have any renames. Just a stale description?

Sorry about comments that should have been on the patch introducing these tests but I hope you can send a separate patch in this series to address this.

260–294

This seems similar to https://www.mercurial-scm.org/repo/hg/file/07b79569fdf8/tests/test-copies.t#l409. Does it turn out to find different bugs in the code?

350–387

Why is this here instead of around line 111? Now I have to scroll back and forth to remind myself what about what the commits here were about. It would have been easier if the tests were just after the setup for that part. That's also how we generally write tests.

(I actually ended up copying the setup and the the verification for each part into an editor just so I could review this patch.)

389–446

This doesn't seem to belong in a file called test-copies*. What's the connection to copies that I'm missing? Don't we have coverage for this already anyway?

marmoute added inline comments.Fri, Mar 6, 5:08 AM
tests/test-copies-chain-merge.t
2

Because these case are broken with these version. I am introducing the #testcases variants once it is fixed. In D8244

8

See previous comment.

13

It is used during debug having the line prevent confusion.

61

That line seems swapped with the previous one. I'll send a patch.

260–294

This is pretty much the same test. In a more systemic effort.

350–387

Because what you suggest cannot be achieve. Since we reuse branches for various cases, we cannot have the tests close to the definition of their branches. I tried, it was confusing and I moved to the organisation currently in core.

389–446

Because the file we delete has copies information. And we track how it could get accessed.

I am not aware of other systemic testing of this.

marmoute added inline comments.Fri, Mar 6, 5:16 AM
tests/test-copies-chain-merge.t
260–294

(also, with more chaining)

martinvonz added inline comments.Fri, Mar 6, 8:26 AM
tests/test-copies-chain-merge.t
350–387

How about setting up the *branches* first then, and then the merges together with the verification?

marmoute added inline comments.Fri, Mar 6, 9:17 AM
tests/test-copies-chain-merge.t
350–387

I tried that too and was not very convinced. Especially now that we have all the test file setup that way I don't see a significant enough difference to be worth the time sink this will be.