Skip to content

Add more consistency tests #4983

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 12 commits into from
Jan 29, 2021
Merged

Conversation

ilevkivskyi
Copy link
Member

Follow up fo #4971

@ilevkivskyi ilevkivskyi requested a review from JukkaL January 28, 2021 19:42
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Might make sense to add a check that all stdlib modules have an entry in VERSIONS

for _, dirs, files in os.walk(directory):
for file in files:
name, ext = os.path.splitext(file)
assert name.isidentifier(), "Files must be valid modules"
Copy link
Member

Choose a reason for hiding this comment

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

Should add the name to the assertion failure to make the failure more debuggable. Same for a number of other checks below.

(Also, is there a reason not to use f-strings here?)

Copy link
Contributor

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests! These will help prevent all sorts weird failure modes that would otherwise be possible, such as breaking the build pipeline.

Looks good, mostly left comments about missing details in error messages.

assert name.isidentifier(), "Files must be valid modules"
assert ext == ".pyi", "Only stub flies allowed. Got: {} in {}".format(file, directory)
for subdir in dirs:
assert subdir.isidentifier(), "Directories must be valid packages"
Copy link
Contributor

Choose a reason for hiding this comment

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

Also include the name of the subdir in the message.


def check_stubs():
for distribution in os.listdir("stubs"):
assert not os.path.isfile(distribution), "Only directories allowed in stubs"
Copy link
Contributor

Choose a reason for hiding this comment

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

Show distribution in the error message.

if ext != ".pyi":
assert entry in {"METADATA.toml", "README", "README.md", "README.rst"}, entry
else:
assert name.isidentifier(), "Bad file name in stubs"
Copy link
Contributor

Choose a reason for hiding this comment

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

Include name in the error message.

if os.path.isfile(os.path.join("stubs", distribution, "@python2", entry)):
name, ext = os.path.splitext(entry)
assert name.isidentifier(), "Bad file name in stubs"
assert ext == ".pyi", "Unexpected file in @python2 stubs"
Copy link
Contributor

Choose a reason for hiding this comment

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

Include file name in the above two error messages.

main()
check_stdlib()
check_stubs()
check_same_files()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also validate METADATA.toml files here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think yes. I will add a test for it too.

Copy link
Contributor

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Can you create a follow-up issue about better dependency tests?

data = toml.loads(f.read())
assert "version" in data, f"Missing version for {distribution}"
version = data["version"]
msg = f"Unsupported Python version{version}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: add space after 'version'?

@ilevkivskyi ilevkivskyi merged commit e2fd852 into python:master Jan 29, 2021
@ilevkivskyi ilevkivskyi deleted the add-consistency-tests branch January 29, 2021 17:23
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.

4 participants