MatthieuDartiailh / bytecode

Python module to modify bytecode
https://bytecode.readthedocs.io/
MIT License
302 stars 38 forks source link

Make test imports relative to tests package #92

Closed juliangilbey closed 2 years ago

juliangilbey commented 2 years ago

The test suite currently has lots of imports of the form from bytecode.tests import .... This means that the tests can only be run when they are present together with the bytecode package itself, not independently. This patch replaces these imports with relative imports: from . import .... All of the tests continue to run successfully with this patch when run in the current way, but they also work when the tests directory is copied elsewhere and the tests directory is removed from the installed package (for the tests are not needed for the bytecode package to work). The only test which does not work when the tests are run on the installed package is test_version, because that requires the presence of setup.py, but that is a minor issue.

codecov-commenter commented 2 years ago

Codecov Report

Merging #92 (81a4a3e) into main (54a076e) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #92   +/-   ##
=======================================
  Coverage   98.52%   98.52%           
=======================================
  Files           7        7           
  Lines        1495     1495           
=======================================
  Hits         1473     1473           
  Misses         22       22           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 54a076e...81a4a3e. Read the comment docs.

P403n1x87 commented 2 years ago

I'd rather move the tests folder one level up, alongside bytecode. Would that still work?

MatthieuDartiailh commented 2 years ago

My opinion on the matter tends to vary on a per project basis, but for such a small library, I like shipping the tests. In addition, if tests were at the top level we could not test against an installed version using pytest due to the presence of an __init__.py which we cannot remove. Otherwise, we would need a src directory but that would mess up the history.

So I am fine with the changes proposed but I am against moving the folder.

juliangilbey commented 2 years ago

You can certainly test against an installed version using pytest, even with __init__.py present. That is exactly what I am doing in my provisional Debian package of bytecode: I install bytecode without the tests directory, then copy the tests directory (with this patch applied) into a temporary directory and run python -m pytest -k "not test_version" tests in that temporary directory. It works perfectly.

MatthieuDartiailh commented 2 years ago

I was considering the simple workflow of just being able to invoke pytest from the root without any complicated dance.

Could you elaborate as to why you find the included tests bothering ?

Also, sorry for the rather late response.

juliangilbey commented 2 years ago

Hi @MatthieuDartiailh! There are two separate things here:

I hope that makes some sense!

MatthieuDartiailh commented 2 years ago

I am fine moving to relative imports, and I will merge shortly. I understand your use. I agree most users won't need the tests, but they get access to utilities used to write them, which can be of interest to them. So at this time I won't move the tests themselves.

Thanks for your contribution.