search

containerbuildsystem / cachi2

GNU General Public License v3.0
7 stars 25 forks source link

add integration tests for pip wheels #504

Closed slimreaper35 closed 2 months ago

slimreaper35 commented 5 months ago

testing repository: https://github.com/cachito-testing/cachi2-pip-wheels

Maintainers will complete the following section

Note: if the contribution is external (not from an organization member), the CI pipeline will not run automatically. After verifying that the CI is safe to run:

slimreaper35 commented 5 months ago

I am not sure about e2e test. The build uses wheels from requirements.txt and requirements-build.txt. If the allow binary option is set to false, the build will still pass because of requirements-build.txt.

Should I remove requirements-build.txt or/and add a scenario where the build fails because of missing requirements-build.txt ? What do you think?

eskultety commented 4 months ago

I am not sure about e2e test. The build uses wheels from requirements.txt and requirements-build.txt. If the allow binary option is set to false, the build will still pass because of requirements-build.txt.

Should I remove requirements-build.txt or/and add a scenario where the build fails because of missing requirements-build.txt ? What do you think?

I think this is the wrong way to think about this. If we purposefully leave out build requirements then it's IMO not a valid e2e test. Instead, I think we need an e2e test that requires allow_binary=true because otherwise it would simply not build at all, so either you'll need to find build requirements that only provide wheels or write an app that depends on a package which only distributes as a wheel.

slimreaper35 commented 4 months ago

I updated the test data so it reflects #508

slimreaper35 commented 4 months ago

Have we covered all of the test cases listed in the story description?

I think so, except hash checking is/isn't used. I just can't find a distribution package, that does not report hashes. If there are no hashes reported by PyPI and no hashes in the requirements.txt => we should download the package without verifying the hash.

ben-alkov commented 4 months ago

Have we covered all of the test cases listed in the story description?

I think so, except hash checking is/isn't used. I just can't find a distribution package, that does not report hashes.

Yep, agreed. AFAUI, it's a baked-in part of the PyPi API - the only way you'd get a request reply without a checksum would be someone spoofing you (or maybe if you're not talking to PyPi but rather some other index, but even then I'm not certain).

If there are no hashes reported by PyPI and no hashes in the requirements.txt => we should download the package without verifying the hash

This should read "we should NOT download the package" - no hashes at all = no download.

eskultety commented 3 months ago

@slimreaper35 I don't see any update to the e2e tests really, so what is your stance on https://github.com/containerbuildsystem/cachi2/pull/504#issuecomment-2036487710 as you haven't replied there really whether you agree or disagree with that argument and this PR then got stuck. From my POV in this case it makes sense having a single e2e test that would test both wheels and sdists at the same time, why? Because fetching is what we really only care about and since we've taken care of testing fetches in different scenarios now we just need to make sure that whatever we fetched can be built with.

slimreaper35 commented 3 months ago

Update (finally):

eskultety commented 3 months ago 
  Should I add test ensuring that pre-fetching will fail on `allow_binary` option set to false ?

Yes we should have such a test. In fact, you had two in earlier revisions, but for some reason you dropped it here: https://github.com/containerbuildsystem/cachi2/compare/ce01911eb42de201fd4d9507ad9d3a46114c23ba..06d216f8de883042e36bbbe47f29a0e452ccf5f8#diff-e4d828af95a3a935bef8644268aee7ff4190648a95b7a401e91a65dbebb9a206L99, any particular reason for that? IIRC I even emphasized the importance of those isolated fail cases here: https://github.com/containerbuildsystem/cachi2/pull/504#discussion_r1550234513