Improve test coverage #32
Conversation
wisp3rwind
force-pushed
the
pr-improve-coverage
branch
from
5dc16e7
to
7e64721
Compare
|
Force-pushed flake8 fixes. |
test/helper.py
Outdated
| @@ -258,6 +278,11 @@ def lib_path(self, path): | |||
| return os.path.join(self.libdir, | |||
| path.replace(b'/', bytestring_path(os.sep))) | |||
|
|
|||
| def item_fixture_path(self, fmt, bytestring=True): | |||
There was a problem hiding this comment.
The bytestring argument is unused. Can we remove it?
There was a problem hiding this comment.
Yes, I thought that I'll need that to construct config values with that path in them, but in the end didn't. Thanks for spotting this!
test/cli_test.py
Outdated
| self.lib_path(b'Michael Jackson/Thriller/track 1.mp3'), | ||
| ) | ||
| by_orig_year_path = self.lib_path(b'by-year/1982/Thriller/track 1.mp3') | ||
| self.assertIsNotSymlink(by_year_path) |
There was a problem hiding this comment.
I think it would make more sense to check that the path does not exist at all. It would be easier to understand than assertIsNotSymlink and we would catch issues when the file still exists but is not a symlink.
There was a problem hiding this comment.
I'm also not really happy with that name. I added an additional assertion, because assertIsFile uses os.path.isfile, which IIRC follows symlinks and stats the target, i.e. if for some reason a symlink ends up being broken, it will not be detected. So if read the documentation correctly, os.path.lexists should precisely check that the path does not exist at all. Which os/os.path method do you think should be used by this test?
There was a problem hiding this comment.
How about renaming assertIsNotSymlink to assertIsNotFile and use not os.path.isfile() and not os.path.islink()?
| @@ -69,7 +69,11 @@ def test_external(self): | |||
| self.assertFalse(os.path.isfile(external_from_ogg)) | |||
| self.assertFileTag(external_beet, b'ISAAC') | |||
|
|
|||
| def test_symlink_view(self): | |||
|
|
|||
| class SymlinkViewTest(TestHelper): | |||
There was a problem hiding this comment.
Docs:
"""Test alternatives with the ``link`` format producing symbolic links.
"""…ing Worker/ThreadPoolExecutor Fakes the conversion process by copying a fixture of another format. The conversion worker will rewrite the tags anyway.
in part courtesy of @geigerzaehler, geigerzaehler#32
|
Going to force-push your additions to the docs + a few more. |
wisp3rwind
force-pushed
the
pr-improve-coverage
branch
from
7e64721
to
0303262
Compare
test/cli_test.py
Outdated
| class ExternalConvertWorkerTest(TestHelper): | ||
| """Test alternatives with non-empty ``format `` option, i.e. transcoding | ||
| some of the files. In contrast to the previous test, these test do not | ||
| work the parallelizing Worker. |
There was a problem hiding this comment.
I think you mistyped. These tests do use the production worker
There was a problem hiding this comment.
Indeed I did...
Improve test coverage
The tests that I added are fairly simple though, but hopefully still better than nothing. Now waiting for Travis...