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

Add new make_wheel test helper #7651

Merged
merged 3 commits into from
Feb 3, 2020

Conversation

chrahunt
Copy link
Member

@chrahunt chrahunt commented Jan 25, 2020

While updating and trying to make our wheel metadata parsing stricter, I noticed
that a large number of our test wheels were noncompliant for various reasons (missing fields, inconsistent metadata).

After a closer look, it's actually pretty bad! The majority of our test wheels are
not automatically generated, many lack documentation, and the ones that are documented
may have documentation in one of several places.

To make it easier to generate compliant-by-default wheels, I've added a helper function
(tests.lib.wheel.wheel_builder) for generating wheel files and toggling various properties of them and a result class (tests.lib.wheel.WheelBuilder) that lets us choose at the call site how we want to consume it.

At a high level this will give us several benefits:

  1. When tests are converted to use it, it will be clear what the difference is from
    a "regular" wheel file
  2. As we increase metadata strictness, we can adjust the default code to align without
    needing a bunch of manual investigation and effort
  3. Since the wheel result doesn't have to hit the filesystem unless asked, this may help
    speed up unit tests that need an actual wheel, but not necessarily a wheel file

To test the completeness of the interface I have locally converted most of our tests, so if this looks OK I can follow up with those changes.

@chrahunt chrahunt added C: tests Testing and related things skip news Does not need a NEWS file entry (eg: trivial changes) labels Jan 25, 2020
@chrahunt chrahunt force-pushed the refactor/wheel-builder-helper-2 branch 2 times, most recently from ed382f5 to 49f40c1 Compare January 25, 2020 17:42
@chrahunt chrahunt marked this pull request as ready for review January 25, 2020 18:18
@chrahunt
Copy link
Member Author

Bump.

@chrahunt chrahunt mentioned this pull request Jan 28, 2020
12 tasks
@chrahunt chrahunt force-pushed the refactor/wheel-builder-helper-2 branch from 49f40c1 to 4f78cc8 Compare January 30, 2020 04:24
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this helper (and PR's title) communicated something very different from what this PR is -- I thought it was adding a helper for the WheelBuilder class.

I suggest renaming the helper to generate_wheel.

@pradyunsg
Copy link
Member

pradyunsg commented Jan 31, 2020

Overall, this sounds like a great idea to me. ^>^


I'd thought of something similar to this, to be a part of the "improve pip test suite" GSoC project ideas -- which tells me that it's probably a good idea, because two people came up with something similar independently. :)

My thought was to include aggregating all the wheels in tests/data and create_*_wheel* helpers, into a much more general generate_wheel function (something similar for source distributions too). Another thing we could explore is creating some sort of "cache" for these generated distributions, to reduce overhead from the generation logic. And potentially move this out into an independently tested helper library. :)

@sbidoul
Copy link
Member

sbidoul commented Jan 31, 2020

I thought it was adding a helper for the WheelBuilder class

@pradyunsg there is no WheelBuilder class anymore ;)

@chrahunt
Copy link
Member Author

I suggest renaming the helper to generate_wheel.

How about make_wheel? Since we'll need to use it a bunch in some places it would be a little easier on the eyes.

@pradyunsg
Copy link
Member

@pradyunsg there is no WheelBuilder class anymore ;)

Which contributed to why I was surprised by the naming here. :)

How about make_wheel?

LGTM.

As we introduce stricter metadata parsing, we will need to ensure that
the wheel files used in our tests are compliant (except in the specific
way that we're testing against).

Currently we have a large number of test cases relying on undocumented or
under-documented wheel files that have various inconsistencies
(incorrect name, missing fields) that are unrelated to the features
actually under test.

With a new wheel builder helper function, we will be able to replace all
of our instances of pre-built wheel test fixtures with dynamically-generated
files in code that are correct by default.
@chrahunt chrahunt force-pushed the refactor/wheel-builder-helper-2 branch from 4f78cc8 to 667dc39 Compare February 1, 2020 00:34
@chrahunt
Copy link
Member Author

chrahunt commented Feb 1, 2020

Updated to make_wheel here.

@chrahunt chrahunt requested a review from pradyunsg February 2, 2020 17:05
@pradyunsg
Copy link
Member

pradyunsg commented Feb 3, 2020

IIUC, we'd want to switch over to this more-and-more in our tests, especially the ones that depend on tests/data/.

@chrahunt
Copy link
Member Author

chrahunt commented Feb 3, 2020

That's the plan. I already have a fair number converted and I can issue PRs for them later today.

@chrahunt chrahunt merged commit f537db5 into pypa:master Feb 3, 2020
@chrahunt chrahunt deleted the refactor/wheel-builder-helper-2 branch February 4, 2020 01:11
@pradyunsg
Copy link
Member

Great!

BTW, @chrahunt: the merge commit for this is the 9000th commit on master. :)

@pradyunsg pradyunsg changed the title Add new wheel builder test helper Add new make_wheel test helper Feb 21, 2020
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Mar 22, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: tests Testing and related things skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants