Page MenuHomePhabricator

copies: split creation of the graph and actual checking again
ClosedPublic

Authored by marmoute on Tue, Oct 13, 6:39 AM.

Details

Summary

The re-install the old split. It will be necessary to test that the upgrade
process produced a functionally identical result. It will be useful to detect
case where the metadata we look at identical, but some other items we did not
checked are missing.

(spoiler: we will find some bug)

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.Tue, Oct 13, 6:39 AM
martinvonz requested changes to this revision.

I really don't like this patch. I did the split back in D8377 because I found this format very hard to read. IIRC, at least @pulkit agreed with me. I don't understand why testing of upgrades require this structure.

This revision now requires changes to proceed.Tue, Oct 13, 11:05 AM

I really don't like this patch. I did the split back in D8377 because I found this format very hard to read. IIRC, at least @pulkit agreed with me. I don't understand why testing of upgrades require this structure.

Because to test the upgrade process, we need all the test-case changeset to be created before we run the upgrade itself.

(spoiler: we will find some bug)

Unable to find the bug in upcoming patches.

I really don't like this patch. I did the split back in D8377 because I found this format very hard to read. IIRC, at least @pulkit agreed with me. I don't understand why testing of upgrades require this structure.

Because to test the upgrade process, we need all the test-case changeset to be created before we run the upgrade itself.

Hm, we can run upgrade in the end then from what I understand.

(spoiler: we will find some bug)

Unable to find the bug in upcoming patches.

The bug is fixed in D9199. Before that patch, the upgrade process was computing copy related sidedata, but failed to set some revlog related flag to mark that changeset was relevant to copy tracing. As a result using an upgraded repository for copytracing returned bad result. When the original test was written, the upgrade worked fine, however, when we later introduced this new flag (for performance reason) the feature silently broke without the test noticing that.

To me this silent breakage highlight the need for more thoughtful testing of the upgrade. We need more that just checking that the upgrade ran without crashing and that some metadata are there. We need to actually check that an upgraded repository can do copy tracing as fine as a repository that was using the feature from the start. That way, we can detect future breackage coming from code/data area we did not expected.

I really don't like this patch. I did the split back in D8377 because I found this format very hard to read. IIRC, at least @pulkit agreed with me. I don't understand why testing of upgrades require this structure.

Because to test the upgrade process, we need all the test-case changeset to be created before we run the upgrade itself.

Hm, we can run upgrade in the end then from what I understand.

My ultimate goal here is to have the tests in this file to run on a upgraded repository. To do so I need the following this to happens in a specific order :

  1. create changeset without copy tracing sidedata
  2. run the repository upgrade to add copy tracing sidedata
  3. check that the copy tracing is yielding that result we want

The upgrade process can only run once. So I need all the changesets created because I run it. Once the upgrade is done, I can check the copy tracing behavior.

Is this clearer to you?

pulkit accepted this revision.Thu, Oct 15, 2:55 PM

I spent some time this afternoon why we cannot just run upgrade in the end and what we want to test. While both me and @martinvonz stills feels like old structure of test was more better and easier to understand, this patch looks fine to me as it increase test coverage related to upgrade and help fix a bug.

This fails to apply on tip of the default branch, kindly rebase and resend (there are 3-4 small conflicts).

I spent some time this afternoon why we cannot just run upgrade in the end and what we want to test. While both me and @martinvonz stills feels like old structure of test was more better and easier to understand, this patch looks fine to me as it increase test coverage related to upgrade and help fix a bug.
This fails to apply on tip of the default branch, kindly rebase and resend (there are 3-4 small conflicts).

The conflict happens in moved code so it always tricky to solve confidently right, but I think I eventually got it right. Sending update now.

marmoute updated this revision to Diff 23229.Thu, Oct 15, 5:06 PM
pulkit accepted this revision.Fri, Oct 16, 3:22 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.