instance create saga: create new DAG for each test iteration #3309
Conversation
Modify the instance create saga's unwind tests so that they create a new saga DAG for every iteration of the test (i.e., for each distinct node in the DAG into which a failure can be injected). This ensures that every test iteration uses a DAG with a unique instance ID, which is needed to ensure that each iteration can actually reach the node into which it intends to inject an error. Add a step to the end of the affected tests that verifies that a previously- created-but-unused DAG can still run to completion after all the error injection tests have been run.
|
|
||
| // Run the saga to completion without injecting any errors to help | ||
| // ensure that an earlier injected failure didn't prevent the saga from | ||
| // failing deterministically. |
There was a problem hiding this comment.
What kind of failure would this be?
There was a problem hiding this comment.
Yeahhh, this comment is word salad. I fixed it in 1c8340f, but I think we can do even better here.
At the risk of being overly pedantic: the problem this test had is that the instance create saga selects its instance ID in SagaInstanceCreate::make_saga_dag and not as part of the saga itself. As previously written, this meant that the test used the same instance ID for all saga executions. That works for a few iterations, but eventually, the test injects a failure after the "create instance record" node. When that node gets unwound for the first time, it leaves behind an instance record bearing the Destroyed state and the reused instance ID. This causes all subsequent saga executions to fail at the "create instance record" node with an "instance already exists" error, which is enough to pass the test (the saga failed!) but isn't what the test wants to do (it wanted the saga to fail at some later node).
The idea here is to run the saga to completion using the "original" DAG just to make sure that the test body didn't reuse the DAG in this way: if it did, then the saga will fail even if no errors were injected.
This was a good quick-and-dirty way to make sure the fix was working as intended, but in retrospect, I think it's kind of squicky, because it assumes so much about the structure of the foregoing test. It would be much better if the test tried to verify directly that the saga failed at the node it expected. I'm trying this out now and think it can be done without too much surgery; will report back when I've run the revised test.
There was a problem hiding this comment.
It would be much better if the test tried to verify directly that the saga failed at the node it expected. I'm trying this out now and think it can be done without too much surgery; will report back when I've run the revised test.
Took a crack at this in 7fac7a1.
Modify the instance create saga's unwind tests so that they create a new saga DAG for every iteration of the test (i.e., for each distinct node in the DAG into which a failure can be injected). This ensures that every test iteration uses a saga that attempts to create an instance with a unique instance ID, which is needed to ensure that each iteration can actually reach the node into which it intends to inject an error.
Add a step to the end of the affected tests that verifies that a previously- created-but-unused DAG can still run to completion after all the error injection tests have been run.