Do you remember my example of a bad stub design in DNF ? At that time I didn't have a good example of why this is a bad design and what are the consequences of it. Today I have!
From my comment on PR #118
Note: the benefit of this patch are quite subtle. I've played around with creating a few more tests and the benefit I see affect only a few lines of code.
For #114 there doesn't seem to be any need to test _get_query directly, although we callq = self.base.sack.query() q = q.available()
which will benefit from this PR b/c we're stubbing out the entire Sack object. I will work on a test later today/tomorrow to see how it looks.
OTOH for #113 where we modify _get_query the test can look something like this:def test_get_query_with_local_rpm(self): try: (fs, rpm_path) = tempfile.mkstemp('foobar-99.99-1.x86_64.rpm') # b/c self.cmd.cli.base is a mock object add_remote_rpm # will not update the available packages while testing. # it is expected to hit an exception with self.assertRaises(dnf.exceptions.PackageNotFoundError): self.cmd._get_query(rpm_path) self.cmd.cli.base.add_remote_rpm.assert_called_with(rpm_path) finally: os.remove(rpm_path)
Note the comment above the with block. If we leave out
_get_queryas before (a simple stub function) we're not going to be able to use
Now a more practical example. See
- in case the package is not found we log the error. In case configuration is
strict=True then the plugin will raise another exception. With the initial version
of the stubs this change in behavior is silently ignored. If there was an error
in the newly introduced lines it would go straight into production because the
existing tests passed.
What happens is that
which is not the real code but a test stub and returns an empty list in this case.
The empty list is expected from the test and it will not fail!
With my new stub design the test will execute the actual
download.py and choke on the exception. The test itself needs
to be modified, which is done in
and no further errors were found.
So let me summarize: When using mocks, stubs and fake objects we should be replacing external dependencies of the software under test, not internal methods from the SUT!