ACCESS-NRI / access-nri-intake-catalog

Tools and configuration info used to manage ACCESS-NRI's intake catalogue
https://access-nri-intake-catalog.rtfd.io
Apache License 2.0
8 stars 1 forks source link

[BUG] `tests/test_data.py::test_get_catalog_fp` fails unless package is installed as editable. #226

Closed charles-turner-1 closed 4 weeks ago

charles-turner-1 commented 1 month ago

Describe the bug

In tests/test_data.py, we have the following test:

def test_get_catalog_fp():
    _oneup = os.path.abspath(os.path.dirname("../"))
    assert str(get_catalog_fp()) == str(
        os.path.join(
            _oneup, "access-nri-intake-catalog/src/access_nri_intake/data/catalog.yaml"
        )
    )

This test passes when run using pytest tests/test_data.py::test_get_catalog_fp in an editable installation (pip install -e .), but fails if access_nri_intake_catalog (pip install .) is installed into site-packages using a regular installation.

To Reproduce

Test passing

$ git clone https://github.com/ACCESS-NRI/access-nri-intake-catalog/ && cd access-nri-intake-catalog 
$ conda create -f ci/environment.yml
$ pip install -e . # ***CRUCIAL DIFFERENCE***
$ pip list | rg intake
access_nri_intake         0.1.4+13.ge90f843.dirty /Users/u1166368/catalog/access-nri-intake-catalog
intake                    0.7.0
...
$ pytest tests/test_data.py
Test session starts (platform: darwin, Python 3.12.6, pytest 8.3.3, pytest-sugar 1.0.0)
rootdir: /Users/u1166368/catalog/access-nri-intake-catalog
configfile: pyproject.toml
plugins: cov-5.0.0, sugar-1.0.0
collected 1 item                                                                                                                                              

 tests/test_data.py ✓                                                                                                                           100%

Results (0.46s):
       1 passed

Test failing

$ git clone https://github.com/ACCESS-NRI/access-nri-intake-catalog/ && cd access-nri-intake-catalog 
$ conda create -f ci/environment.yml
$ pip install .  # ***CRUCIAL DIFFERENCE***
$ pip list | rg intake
access_nri_intake         0.1.4+13.ge90f843.dirty
intake                    0.7.0
...
$ pytest tests/test_data.py
Test session starts (platform: darwin, Python 3.12.6, pytest 8.3.3, pytest-sugar 1.0.0)
rootdir: /Users/u1166368/catalog/access-nri-intake-catalog
configfile: pyproject.toml
plugins: cov-5.0.0, sugar-1.0.0
collected 1 item                                                                                                                                              

――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― test_get_catalog_fp ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――

    def test_get_catalog_fp():
        _oneup = os.path.abspath(os.path.dirname("../"))
>       assert str(get_catalog_fp()) == str(
            os.path.join(
                _oneup, "access-nri-intake-catalog/src/access_nri_intake/data/catalog.yaml"
            )
        )
E       AssertionError: assert '/Users/u1166.../catalog.yaml' == '/Users/u1166.../catalog.yaml'
E         
E         - /Users/u1166368/catalog/access-nri-intake-catalog/src/access_nri_intake/data/catalog.yaml
E         ?                 ^^^^^                    ^^^ ^^ ^  ^
E         + /Users/u1166368/miniforge3/envs/access-nri-intake/lib/python3.12/site-packages/access_nri_intake/data/catalog.yaml
E         ?                 ^^^^^ + +++++++                  ^^^^^^^ ^ ^^^^^  ^^^^^^ +++++

tests/test_data.py:11: AssertionError

 tests/test_data.py ⨯                                                                                                                           100% ██████████/Users/u1166368/miniforge3/envs/access-nri-intake/lib/python3.12/site-packages/coverage/control.py:894: CoverageWarning: No data was collected. (no-data-collected)
  self._warn("No data was collected.", slug="no-data-collected")

---------- coverage: platform darwin, python 3.12.6-final-0 ----------
Coverage XML written to file coverage.xml

=================================================================== short test summary info ===================================================================
FAILED tests/test_data.py::test_get_catalog_fp - AssertionError: assert '/Users/u1166.../catalog.yaml' == '/Users/u1166.../catalog.yaml'

Results (0.52s):
       1 failed
         - tests/test_data.py:9 test_get_catalog_fp

Additional context

This bug is arising because we've (in essence - there are some subtleties here) hardcoded paths to respect an editable install - when using a src layout the package structure changes when pip installed.

Editable installs are not a perfect replacement for regular installs in a test environment. When in doubt, please test your projects as installed via a regular wheel. There are tools in the Python ecosystem, like tox or nox, that can help you with that (when used with appropriate configuration).

@marc-white It looks to me like you wrote this test - do you know if you had to change the CI to an editable install to get this test to pass on CI or whether it was already set to editable?

I also think bugs like this might put us in favour of using a flat over src layout as an organisational standard - this sort of issue is less common with a flat layout as setuptools wont restructure the package when it builds it (@rbeucher @aidanheerdegen maybe one for the python standards discussion).

marc-white commented 1 month ago

@charles-turner-1 I think I already had an editable install without realizing.

I think the test can probably be fixed by changing how _oneup is defined, possibly by referencing the path of the test file itself and constructing a relative path from there? The aim of the test is to make sure the tool.setuptools.package-data from the pyproject.toml is being accessed correctly.

charles-turner-1 commented 1 month ago

I fixed a couple of packaging issues like this in my last job & from memory it's quite a bit more fiddly to resolve properly than I'd expected. It looks to me like we don't actually have a MANIFEST.in in the repo so it's somewhere between vaguely possible & quite likely that we've been building the catalogue using an editable install - which would explain why we hadn't noticed this as an issue - although it introduces reproducibility concerns.

EDIT: looks like tool.setuptools.package-data negates the need for a MANIFEST.in & the data are correctly included in the built package so ignore those comments.