Skip to content

Centralize METADATA.toml parsing in the test suite #9534

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

Merged
merged 10 commits into from
Jan 28, 2023

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jan 14, 2023

We have various bits of logic scattered across the test suite to parse METADATA.toml files. This is a bug magnet: see e.g. #9532 (comment). (Failing CI run here: https://github.com/python/typeshed/actions/runs/3918898414/jobs/6699564673.) By centralizing the logic all in one place, we reduce the risk of bugs in our tests, and also improve type safety.

This PR centralizes METADATA.toml-parsing logic into a new file, tests/parse_metadata.py.

@@ -162,46 +130,8 @@ def _find_stdlib_modules() -> set[str]:

def check_metadata() -> None:
Copy link
Collaborator

@Avasam Avasam Jan 14, 2023

Choose a reason for hiding this comment

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

At this point, this feels redundant since other tests will fail anyway. 😅
Unless you wanna use it as a smoke test.

Copy link
Member Author

@AlexWaygood AlexWaygood Jan 14, 2023

Choose a reason for hiding this comment

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

I get what you mean, but the other tests will assert the correctness of the METADATA.toml files "by accident". I feel like it's useful to have a test that asserts the correctness of the METADATA.toml files "on purpose"? That way we still have this guaranteed to be tested in CI, even if some other tests no longer need to get stuff from METADATA.toml files in the future, for whatever reason.

Copy link
Collaborator

@Avasam Avasam Jan 14, 2023

Choose a reason for hiding this comment

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

Yeah that's what I meant by a smoke test (an easy test that says, if this fails, everything else will fail anyway, so it's easy to find the source of the error). Just wanted to know if it was on purpose :)

@Avasam
Copy link
Collaborator

Avasam commented Jan 17, 2023

@AlexWaygood
Copy link
Member Author

What about the [mypy-tests] sections? https://github.com/python/typeshed/pull/9534/files#diff-b42eb36bcaa2d1a163ed1c202e81cffd058915ca695b660d49c636b3a5b56857R199-R216 (which doesn't seem to be currently used anywhere?)

I believe that if somebody did try to add a per-stub config section for mypy, check_consistent.py would already complain about it. And, as you say, it's not currently used anywhere. I'd prefer not to touch it for this PR.

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

Successfully merging this pull request may close these issues.

2 participants