I don't think use of files is that helpful -- the CSI does very few
file manipulations directly since it is designed around some of the
more advanced options of `fallocate` called through the shell -- but
doing this exposes something interesting about some refactoring that
would be useful to improve the code's test robustness. It also makes
the tests noticeably slower.
I'm not going to do those changes now, because we have some other
project priorities and at least as-is the tests fumble through all the
lines and branches of this implementation by brute force, which is
good enough for now. But I wanted to describe it for later reference
and education:
The "unit" for `perform_node_stage_volume` is too monolithic. It
starts innocently at first:
it "handles fallocate failure" do
prefix_helper.call do |service, req, backing_file|
expect(service).to receive(:run_cmd).with("fallocate", "-l", size_bytes.to_s, backing_file, req_id:).and_return(["Error message", false])
expect { service.perform_node_stage_volume(req_id, pvc, req, nil) }.to raise_error(GRPC::ResourceExhausted, /Failed to allocate backing file/)
This is because `fallocate` is the first step in a multi-step process.
However, once we want to test the second step, we need to keep some
kind of `fallocate` mock to then inject faults in the second step:
prefix_helper.call do |service, req, backing_file|
expect(service).to receive(:run_cmd).with("fallocate", any_args, req_id:).and_return(["", true]).at_least(:once)
expect(service).to receive(:run_cmd).with("losetup", "-j", any_args, req_id:).and_return(["a fake found loop device", true])
expect(service).to receive(:run_cmd).with("losetup", "--find", "--show", backing_file, req_id: req_id).and_return(["Error setting up loop device", false])
This keeps building as we want to proceed deeper into the code.
The general idea to fix this would be to refactor the implementation
so that each error-prone step can have its faults injected in detail,
but they fit together into a somewhat simple data flow in the end.
That data flow has to be simple, for two reasons:
* If it's complex, the interaction of mocks and what the resulting
mocked code can prove about the implementation is hard to read.
* If it's complex, the tests are more voluminous and greater in
quantity to handle various branches, and are longer.
Sometimes, completely satisfying solutions are not available for every
piece of code, but I think we could succeed here, if we had time to.
2baac19a82