RSDK-10762 Add test for optional dependency cycles #5039
Conversation
viambot
added
the
safe to test
| return nil | ||
| } | ||
|
|
||
| func TestModularOptionalDependenciesCycles(t *testing.T) { |
There was a problem hiding this comment.
I think this test is about having two resources, "A" and "B". Where "A" optionally depends on "B" and "B" optionally depends on "A"?
It's a good test name, but I still think it's worth just writing that out in plain english as a test comment. And stating what the behavior we're asserting on. I assume this is a legal config and we expect A and B both (eventually) get reconfigured with handles to the other one.
But because of the cycle I expect this has to work something like:
- Create A (without B)
- Create B (with A)
- Reconfigure A with B
Or vice versa where B gets constructed first. Or, if A and B are constructed concurrently:
- Create A (without B) while also Creating B (without A)
- Reconfigure A with B
- Reconfigure B with A
I think at least describing the expected/allowable interleavings would be useful for understanding a future test failure.
There was a problem hiding this comment.
Your description of the test is pretty much accurate, yep. Good idea to have an explanatory comment; added. I also renamed the test to TestOptionalDependenciesCycle since there's nothing modular going on here.
expected interleavings
I believe the ordering of reconfigures and constructions are deterministic with this test. That is because I am not adding both A (moc) and B (moc2) at the same time. I can add A and B simultaneously if we want to test that. With what I'm doing now, the order should always be the first interleaving you described:
- Construct A (without B)
- Construct B (with A)
- Reconfigure A with B
Plus two extra steps:
- Remove A
- Reconfigure B without A
| return nil | ||
| } | ||
|
|
||
| func TestModularOptionalDependenciesCycles(t *testing.T) { |
There was a problem hiding this comment.
Your description of the test is pretty much accurate, yep. Good idea to have an explanatory comment; added. I also renamed the test to TestOptionalDependenciesCycle since there's nothing modular going on here.
expected interleavings
I believe the ordering of reconfigures and constructions are deterministic with this test. That is because I am not adding both A (moc) and B (moc2) at the same time. I can add A and B simultaneously if we want to test that. With what I'm doing now, the order should always be the first interleaving you described:
- Construct A (without B)
- Construct B (with A)
- Reconfigure A with B
Plus two extra steps:
- Remove A
- Reconfigure B without A
| return moc, nil | ||
| } | ||
|
|
||
| func (moc *mutualOptionalChild) Reconfigure(ctx context.Context, deps resource.Dependencies, |
There was a problem hiding this comment.
Discussed offline: you must have a reconfigurable resource to leverage optional dependencies at all. This is not something that was captured in documentation, so I'll have to chat with NetCode/docs about this.
viambot
added
safe to test
| // | ||
| // A resource 'moc' will optionally depend upon 'moc2', and 'moc2' will optionally | ||
| // depend upon 'moc'. We will start with only 'moc' in the config, and assert that 'moc' | ||
| // builds successfully without 'moc2'. We will then add 'moc2' to the config, assert |
There was a problem hiding this comment.
We will then add 'moc2' to the config, assert that 'moc' reconfigures successfully, 'moc2' builds successfully, and both resources have handles to each other.
Does moc2 only have its constructor invoked and be successful? Or is the construction immediately followed by a Reconfigure?
I assume only the constructor is called. So moc2 will get its optional dependency via that (thanks to moc[1] already existing). But if moc2 did not have a Reconfigure, it would never be able to learn of changes to moc[1].
edit
Ok, now I see the bit further in the test about weak resources.
There was a problem hiding this comment.
I assume only the constructor is called. So moc2 will get its optional dependency via that (thanks to moc[1] already existing). But if moc2 did not have a Reconfigure, it would never be able to learn of changes to moc[1].
That sounds correct to me!
RSDK-10762
Adding some quick test coverage for optional dependency cycles, since we have had issues in the past with weak dependency cycles.