Test "kubectl logs" #9
Conversation
| impl<'a, T: DeletableResource> Drop for TemporaryResource<'a, T> { | ||
| fn drop(&mut self) { | ||
| let resource = mem::take(&mut self.resource); | ||
| self.client.delete(resource); |
There was a problem hiding this comment.
This will panic if the resource was deleted before the struct goes out of scope.
Should we be more forgiving here, as I understand this to mostly be a helper struct that cleans up.
Any actual assertions about the objects existance or content should be part of the test cases, no?
There was a problem hiding this comment.
It should be impossible to delete the resource elsewhere in the code. The resource could be deleted by the user with kubectl delete while the test is running but then a what-are-you-doing?-panic is okay.
There was a problem hiding this comment.
I'm not sure about that, to me that is just plumbing. As long as the test case finished successfully, all relevant assertions should have been made and fulfilled in there. Why would we care if the resource is gone when we try to clean it up?
But as I said, happy for this to stay as it is as well. Time will tell if it ever even becomes relevant, then we can revisit this with more context if needed.
| "#}); | ||
| setup_repository(&client); | ||
|
|
||
| let pod = TemporaryResource::new( |
There was a problem hiding this comment.
You have removed the check for an existing object - which I understand to be due to the new usage of TemporaryResource - but I'm fairly sure there are scenarios where cleanup might fail.
Would it perhaps make sense to add the check for and removal of old objects to TemporaryObject? Could be split up in new() and new_and_remove_if_exists() or something similar in case a test ever needs the distinction..
There was a problem hiding this comment.
This was also my first thought. But if it is not possible to delete a resource for the first time why should it be possible to delete it the second (or third) time? Sure, time passes and the system could have changed to allow the deletion meanwhile, but when do you stop trying?
In the former code the resource was not deleted if the test case failed/panicked. So it was necessary to do the check. With the introduction of TemporaryResource I never encountered this problem anymore.
One remaining problem is that system signals like SIGINT are not handled. If the user presses Ctrl+C while the tests are running, chances are high that pods remain. As the pods often have unique names with UUIDs included, the former existence check would not help. It would be necessary to implement correct signal handling, e.g. with the ctrlc crate.
There was a problem hiding this comment.
The main scenario that I considered was network trouble that makes Kubernetes briefly unavailable during the test which causes the test to fail without removing the resource.
Same as with the comment above though, happy to leave as is and see if it ever becomes an issue.
|
|
||
| /// Newline character for LOG_OUTPUT | ||
| /// | ||
| /// Source code: \\\\\\\\n |
There was a problem hiding this comment.
Thanks for documenting this, I can only imagine the pain that went into these five lines!
There was a problem hiding this comment.
Some of the layers which eat backslashes are under our control. We could try to make them less voracious.
There was a problem hiding this comment.
Probably not worth the effort unless it creates issues.
|
Work is continued in branch test_logs_with_t2 |
Closes stackabletech/agent#37