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
Branch
snapshot-strategy-no-alias-cycles
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9466
Build 13905: tox-on-jenkinsJenkins
Build 13904: arc lint + arc unit

Event Timeline

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
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.

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)

Thanks for the investigation!

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