test: cram tests are run twice when both aliases are built #11547
Conversation
Alizter
force-pushed
the
bug-cram-double-run
branch
from
9d900a5 to
82cb466
Compare
|
Actually this fix isn't great. The issue is that cram tests collect all the aliases together in a set and I have to manually pick them out. This causes indeterminacy observed in the test suite. I'll need to do some more refactoring to achieve the desired determinacy. |
Alizter
force-pushed
the
bug-cram-double-run
branch
from
82cb466 to
bfbd121
Compare
|
I've had to touch a bit more than I hoped, but this seems like it will work better. I'm going to wait and see what the CI will say. I added an extra field to the cram test specification which is the "chosen alias". When crawling for cram tests and updating empty specifications, we make sure to always set the name of the cram test as the main alias. Later when we set up the rule for the cram tests we make sure to never iterate over all aliases and instead add rules for the main alias. That way we can add secondary rules for extra aliases depending on the main alias. This means that the anonymous action we are creating won't ever be run twice which was the original bug. I've also updated the test to describe the expected / observed behaviour. |
|
Looks like I triggered an edge case which I didn't think would happen with this refactoring. I'll investigate and update the fix shortly. |
|
The issue was that the initial cram specification was wrong. I made it so that only the main alias is set. I'll squash this once the CI is green. |
|
@maiste I think this is ready for another review. I'll squash the fix after you are done so you can see some of the reasoning behind the changes. |
Alizter
force-pushed
the
bug-cram-double-run
branch
2 times, most recently
from
f53188e to
bbb2583
Compare
This bug was noticed when working on ocaml#11109. The issue is that the cram rules create two copies of the anonymous cram action, however since the alias names are different (runtest vs cram test name) this ends up with having two different digests. This means that when building `@runtest` and `@mycram` we end up running the cram test `mycram.t` twice. This test demonstrates this behaviour. Signed-off-by: Ali Caglayan <alizter@gmail.com>
Alizter
force-pushed
the
bug-cram-double-run
branch
from
bbb2583 to
9bc6d41
Compare
| let* () = | ||
| Memo.parallel_iter extra_aliases ~f:(fun extra_alias -> | ||
| Rules.Produce.Alias.add_deps ~loc extra_alias (Action_builder.dep (Dep.alias alias))) | ||
| in |
There was a problem hiding this comment.
Rather than use Super_context.add_alias_action I opted to directly register the dependency with Rules.Produce.Alias.add_deps.
There was a problem hiding this comment.
When I had to register an alias, I encountered these two options. What are the difference between them? My bet would be on the way they operate and that one call the other one, but I have always wondered if there was a behavior difference under the hood.
There was a problem hiding this comment.
They are similar but not exactly the same. Super_context.add_alias_action uses Rules.Produce.Alias.add_action which creates an anonymous action with that alias. On the other hand, Rules.Produce.Alias.add_deps just adds a dependency on the given unit Action_builder.t which could be another alias as it is in this situation.
This is all very confusing, but the bread-and-butter is that the anonymous action has a digest that takes into account the name of the alias rather than just depending on it. This means that if we are iterating over multiple aliases with the same action, we would have multiple different actions. But if we instead registered them as deps, then we would have a single action with all of these depending on it.
|
I'll stop pushing for now, sorry about the bumps. I was a bit rusty on the codebase so I didn't find everything I needed the first time round. This time everything should be ready. |
| let* () = | ||
| Memo.parallel_iter extra_aliases ~f:(fun extra_alias -> | ||
| Rules.Produce.Alias.add_deps ~loc extra_alias (Action_builder.dep (Dep.alias alias))) | ||
| in |
There was a problem hiding this comment.
When I had to register an alias, I encountered these two options. What are the difference between them? My bet would be on the way they operate and that one call the other one, but I have always wondered if there was a behavior difference under the hood.
We fix an issue where cram tests were being run twice due to anonymous actions being created for each alias associated to a given cram test. We fix this by distinguishing one of the aliases and only creating the action for that while allowing the other aliases to depend on that alias. This required changing the cram specification slightly, but has simplified the rule generation somewhat. Signed-off-by: Ali Caglayan <alizter@gmail.com>
Alizter
force-pushed
the
bug-cram-double-run
branch
from
9bc6d41 to
837d2e5
Compare
In the first commit we demonstrate a bug with cram tests.
This bug was noticed when working on #11109. The issue is that the cram
rules create two copies of the anonymous cram action, however since the
alias names are different (runtest vs cram test name) this ends up with
having two different digests. This means that when building
@runtestand@mycramwe end up running the cram testmycram.ttwice.This test demonstrates this behaviour.
The second commit in this PR fixes this bug.