Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix generic importer tests that implicitly depend on libmagic #74

Merged
merged 1 commit into from Dec 5, 2020

Conversation

carljm
Copy link
Collaborator

@carljm carljm commented Dec 5, 2020

The generic importer source takes a directory, looks at all files in
that directory recursively, and then filters down to the files that are
of the right type for its importer. The tests for it point it to a
directory that contains not only the test source CSV file, but also the
test verification beancount and JSON files.

I haven't verified, but I'm guessing this worked for the original author
because they had python-magic and libmagic installed. But without
those installed, beancount's file-identification fails (ironically) on
the .beancount files, causing these tests to fail.

Since libmagic is supposed to be an optional dependency of beancount,
and it can't be listed as an automatic test dependency (since it's not a
Python library), the tests should not require it in order to pass.

This behavior of the generic importer (silently excluding files of a
type it doesn't know how to handle) seems a bit too magical and implicit
for my tastes, but since I didn't author and don't use this importer I'm
not changing its behavior. Instead I just rearranged the test data
directory so the source CSV file is in its own subdirectory, eliminating
the problem of making the importer identify the various other test
support files.

With this PR all tests are green again, even without libmagic
installed.

The generic importer source takes a directory, looks at all files in
that directory recursively, and then filters down to the files that are
of the right type for its importer. The tests for it point it to a
directory that contains not only the test source CSV file, but also the
test verification beancount and JSON files.

I haven't verified, but I'm guessing this worked for the original author
because they had `python-magic` and `libmagic` installed. But without
those installed, beancount's file-identification fails (ironically) on
the `.beancount` files, causing these tests to fail.

Since `libmagic` is supposed to be an optional dependency of beancount,
and it can't be listed as an automatic test dependency (since it's not a
Python library), the tests should not require it in order to pass.

This behavior of the generic importer (silently excluding files of a
type it doesn't know how to handle) seems a bit too magical and implicit
for my tastes, but since I didn't author and don't use this importer I'm
not changing its behavior. Instead I just rearranged the test data
directory so the source CSV file is in its own subdirectory, eliminating
the problem of making the importer identify the various other test
support files.

With this PR all tests are green again, even without `libmagic`
installed.
@carljm
Copy link
Collaborator Author

carljm commented Dec 5, 2020

cc @dumbPy, author of the tests in question here

@coveralls
Copy link

coveralls commented Dec 5, 2020

Pull Request Test Coverage Report for Build 173

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-67.04%) to 0.0%

Totals Coverage Status
Change from base Build 162: -67.04%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

1 similar comment
@coveralls
Copy link

coveralls commented Dec 5, 2020

Pull Request Test Coverage Report for Build 173

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-67.04%) to 0.0%

Totals Coverage Status
Change from base Build 162: -67.04%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

@jbms jbms merged commit 77e97d2 into jbms:master Dec 5, 2020
@jbms
Copy link
Owner

jbms commented Dec 5, 2020

Thanks!

@carljm carljm deleted the fix-tests branch December 6, 2020 00:19
@dumbPy
Copy link
Contributor

dumbPy commented Dec 19, 2020

Hi @carljm
Thank you for this fix. You are right, I use anaconda so I didn't actually check the dependencies.

This behavior of the generic importer (silently excluding files of a
type it doesn't know how to handle) seems a bit too magical and implicit
for my tastes, but since I didn't author and don't use this importer I'm
not changing its behavior.

I am actually using beancount.ingest.importers.csv.Importer in the test and the file filtering is actually baked into beancount and with that, this implicit behaviour, I suppose. Nonetheless, Thank you for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants