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

Testing as-installed package downstream #40

Closed
bollwyvl opened this issue Oct 25, 2022 · 22 comments
Closed

Testing as-installed package downstream #40

bollwyvl opened this issue Oct 25, 2022 · 22 comments
Labels
enhancement New feature or request

Comments

@bollwyvl
Copy link
Contributor

Thanks for this package!

It would be lovely for downstream packagers if the tests:

  • made it through in the sdist on PyPI
    • and still wouldn't get installed
  • used import jsf rather than import ..jsf so that they could test the as-installed package

I'd be happy to work up a PR that did these things, if that was desirable.

Motivation: I'm looking to package this for conda-forge:

The lack of tests aren't a hold-up, but do help us catch metadata creep which is only semi-automated.

Thanks again!

@ghandic
Copy link
Owner

ghandic commented Oct 25, 2022

Sure! PR welcome :)

I would like to change the packaging to use pants and add some additional build automation to the pipeline - just not seeming to find enough time this month to sit down and make it happen!

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Jan 2, 2023

Looks like we lost LICENSE in the most recent release

@ghandic
Copy link
Owner

ghandic commented Jan 2, 2023

Are the rest of the requested features now archived? Or is there something else?

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Jan 2, 2023

The tests are also still not included in the .tar.gz, so didn't look at running them.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Jan 2, 2023

Also the PyPI index page contains no long description.

@ghandic
Copy link
Owner

ghandic commented Jan 2, 2023

I know about the long description - was willing for that to be parked for now, plan was to symlink it.

Might need to do the same with the license - is that required??

For the tests, pants will never package these as they are not a dependency of the target. I thought conda-forge could have its own way for build and test though - so can it not clone this repo itself and run make test, make build

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Jan 3, 2023

Yeah, willing to work around the stuff mentioned (e.g. already fetching the LICENSE), but I believe the point of an sdist is to do most of these things, irrespective of how it was arrived at, and then rebuilt with a minimal amount of tooling, on any platform, ideally with just python.

do the same with the license - is that required??

LICENSE says:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

clone this repo itself and run make test

GitHub-generated tarballs are not stable over time, and VCS tags can be moved: having a canonical source distribution on PyPI allows for more thorough provenance. PyPI tarballs can be yanked, of course, but that's usually a strong indicator that something is really broken... but folk don't usually delete their broken VCS tags.

@ghandic
Copy link
Owner

ghandic commented Jan 7, 2023

LICENSE, long description added in - #52

Will make a separate PR for tests, need to think it through more...

@ghandic
Copy link
Owner

ghandic commented Jan 7, 2023

See #53 for testable sdist

@ghandic
Copy link
Owner

ghandic commented Jan 7, 2023

If this is as expected, then I'm happy to merge and include in the next release.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Jan 7, 2023

Thanks for the update. Downstream, still seeing one missing test file in the sdist, but it's easy to skip a single test for now:

>       return _builtin_open(local_path, mode, buffering=buffering, **open_kwargs)
E       FileNotFoundError: [Errno 2] No such file or directory: 'jsf/tests/data/external-ref-common.json'

../_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/lib/python3.11/site-packages/smart_open/smart_open_lib.py:363: FileNotFoundError
=============================== warnings summary ===============================
../_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/lib/python3.11/site-packages/rstr/xeger.py:2
  $PREFIX/lib/python3.11/site-packages/rstr/xeger.py:2: DeprecationWarning: module 'sre_parse' is deprecated
    import sre_parse

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/test_default_fake.py::test_external_ref - FileNotFoundError: [Er...
=================== 1 failed, 70 passed, 1 warning in 3.05s ====================

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Jan 7, 2023

Actually, looks like the file is in the sdist, it's just not found during test.

@ghandic
Copy link
Owner

ghandic commented Jan 7, 2023

Interesting... I have added a test in the pipeline to check that the sdist tests work by unzipping, cd into new directory and then running pytest. All works fine in the pipes

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Jan 7, 2023

yes, that would work as the paths are all relative. The approach we generally use is:

pip install .
# get a clean env with the package installed, and a clean working folder
pytest -m jsf

copying only that file to ./jsf/tests/data/ allows the test to pass against the as-installed package, however, other things (like coverage) don't work, as ./jsf shadows {sys.prefix}/lib/python3.10/site-packages/jsf for its discovery purposes.

Again, the answer can just be "testing the as-installed packages is not supported," if this is becoming a hassle.

@ghandic
Copy link
Owner

ghandic commented Jan 8, 2023

make build && tar -xvf dist/jsf-0.5.4.tar.gz
cd jsf-0.5.4
pip install .
pytest -m jsf
========================================= test session starts =========================================
platform darwin -- Python 3.9.13, pytest-7.2.0, pluggy-1.0.0
rootdir: /Users/andrewchallis/Documents/jsf/jsf/jsf-0.5.4
plugins: Faker-15.3.4
collected 71 items / 71 deselected / 0 selected                                                       

======================================= 71 deselected in 0.26s ========================================

I'm a little confused as to the expectation... is it to cd into the site-packages/jsf and run the tests from there?

@ghandic
Copy link
Owner

ghandic commented Jan 8, 2023

eg

cd /Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/site-packages/jsf
pytest

Then I get 1 failure

@ghandic
Copy link
Owner

ghandic commented Jan 8, 2023

Updated pipe to test as such, and changed the remote test to use a remote file through HTTP back to this same repo. #57

@ghandic ghandic added the enhancement New feature or request label Jan 8, 2023
@ghandic
Copy link
Owner

ghandic commented Jan 8, 2023

Only thing to be consious of, is the testing requirements - currently just pytest and pyjwt, not sure how conda suggests you manage that?

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Jan 8, 2023

remote file through HTTP back to this same repo

that's fine, i suppose.

When dealing with remote resources in schema, it's also possible to use a jsonschema.validators.RefResolver, which can be told something is in a particular place.

It's also possible to use e.g. Path(some_file).as_uri() to get back the fully-qualified absolute file:/// in a platform-independent way, but that may not be appropriate here. Note that github does rate limit, but if it's onesy-twosy, it shouldn't be a big deal.

how conda suggests you manage that?

The top-level structure of the build description has a requirements section for build (e.g. compilers), host (e.g. pip), run and test, as can be seen on the 0.7.0 PR. Note it's currently blocked by the rstr upstream to get a compatible version, but otherwise works.

@ghandic
Copy link
Owner

ghandic commented Jan 9, 2023

Awesome, good to close this issue? Can reopen if theres other things found :)

@bollwyvl
Copy link
Contributor Author

Sorry for the delay: just shipped conda-forge/jsf-feedstock#6 with tests and licenses sourced from the sdist, and no patches 🎉 ! Thanks again.

@ghandic
Copy link
Owner

ghandic commented Jan 14, 2023

Awesome! Is it a process that's automated for future releases? Or will someone need to raise PR's on feedstock?

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

No branches or pull requests

2 participants