Make the Bringer better testable #240
Conversation
- Extract wait for network to a separate utility function - Delegate wrapping of a fetch by thread to a method of Bringer - Remove the now-empty intermediate fetching helper functions from data_fetch
matejak
added
port-RHEL8
There was a problem hiding this comment.
So far it works great for me because I tried to build the update image and run the Anaconda installation and changed various SCAP content in the Security Policy Scope.
However, I have found an issue, which isn't caused by the contents of this PR: #244
| fetch_data_thread = AnacondaThread( | ||
| name=fetching_thread_name, | ||
| target=self.fetch_operation, | ||
| args=(self.content_uri, self.dest_file_name, ca_certs_path), |
There was a problem hiding this comment.
I have noticed a weird thing that was there before but I think we can talk about it nevertheless. The URI is a property of the class (self.content_uri) but the certificate path isn't a property of the class but a variable (ca_certs_path) and is passed down. But, both are arguments of the fetch_content content method. So the arguments of fetch_content are each handled differently at this moment.
There was a problem hiding this comment.
Good point, the common argument to a generl fetch operation would be the URI that we store in the class anyway, and then other arguments depend on how we fetch, certs being needed only if we fetch from network using an "s" protocol.
Therefore, it is cleaner to store those variable arguments in the instance as well.
| return fetching_thread_name | ||
|
|
||
| def finish_content_fetch(self, fetching_thread_name, fingerprint): | ||
| def fetch_operation(self, uri, out_file, ca_certs_path=None): | ||
| return data_fetch.fetch_data(uri, out_file, ca_certs_path) |
There was a problem hiding this comment.
What is the reason for having this wrapper function? Can we simply move code from the data_fetch.fetch_data to fetch_operation?
There was a problem hiding this comment.
The other function is somewhat coupled with the data_fetch module - the way it is unit-tested as well as the way exceptions it can raise are defined.
The whole data_fetch module could be merged into the bringer in the future, but I think that it is OK to have such thin wrapper.
The fetch operation as a separate method is convenient to be able to override it in child classes e.g. for testing purposes.
- Make ca_certs a class attribute - Don't pass those items as method arguments all the time, simply reference them from the instance.
|
The inspection completed: 5 updated code elements |
Description:
This PR moves functionality formerly located in the data_fetch module to the Bringer, so one can use polymorphism to create great mocks by just subclassing.
Now, it is possible to test handling of threads and fetch methods separately, because Bringer just calls a method to fetch a URI, and then wraps the process in a thread.
Rationale:
Review Hints: