Page MenuHomeSoftware Heritage

hypothesis_strategies/snapshots: Explain last post-processing step
ClosedPublic

Authored by anlambert on Dec 2 2019, 2:31 PM.

Details

Summary

Hypothesis may generate cycles between branch aliases in snapshots strategy so
prevent to encounter such cases in tests using it.

Clarify the last post processing step of the snapshots strategy by adding a comment.

Diff Detail

Repository
rDMOD Data model
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

anlambert created this revision.Dec 2 2019, 2:31 PM
anlambert updated this revision to Diff 8392.Dec 2 2019, 3:21 PM

Update: Need to test against 'alias' instead of TargetType.ALIAS to avoid hypothesis.errors.Flaky

olasd requested changes to this revision.Dec 2 2019, 6:45 PM
olasd added a subscriber: olasd.

Cycle detection in snapshots is supposed to be handled in the snapshot_identifier function; that's why it's called in a loop when generating the snapshot id.

If there's a bug here it should be fixed in that function (and a regression test should be added).

This revision now requires changes to proceed.Dec 2 2019, 6:45 PM
olasd added a comment.Dec 2 2019, 6:46 PM
In D2383#56176, @olasd wrote:

Cycle detection in snapshots is supposed to be handled in the snapshot_identifier function; that's why it's called in a loop when generating the snapshot id.

If there's a bug here it should be fixed in that function (and a regression test should be added).

We should also add a comment in the id generation to explain that we're using the cycle detection feature, because that's not obvious at all here.

In D2383#56176, @olasd wrote:

Cycle detection in snapshots is supposed to be handled in the snapshot_identifier function; that's why it's called in a loop when generating the snapshot id.

If there's a bug here it should be fixed in that function (and a regression test should be added).

Oh I see, I need to dig further on this then.

We should also add a comment in the id generation to explain that we're using the cycle detection feature, because that's not obvious at all here.

Indeed, I thought it was related to another error I did not know of. I will update accordingly.

If there's a bug here it should be fixed in that function (and a regression test should be added).

Turns out I can not reproduce anymore the cycles issue I encountered so I must have introduced it by hacking on the strategy implementation.

Initially I stumbled across aliases chain in the snapshot content generated by hypothesis that was making a test failed for swh-web
as it does not handle such cases (fixed in D2384). I ended up getting alias cycles somehow in the generated data but I must have messed
up somewhere.

So I will simply update that diff to add the comment about the cycles detection in the strategy.

anlambert updated this revision to Diff 8401.Dec 2 2019, 8:00 PM

Update: There was no cycles generation between aliases issue after all, probably myself that messed up somewhere
when playing with hypothesis.

So modify that diff to simply add a comment in the strategy implementation to indicate the last post processing step
will remove any generated cycles if any.

anlambert retitled this revision from hypothesis: Forbid branch alias cycles generation in snapshots strategy to hypothesis_strategies/snapshots: Explain last post-processing step.Dec 2 2019, 8:02 PM
anlambert edited the summary of this revision. (Show Details)
olasd accepted this revision.Dec 3 2019, 10:35 AM

Thanks for the investigation!

This revision is now accepted and ready to land.Dec 3 2019, 10:35 AM