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 CI test for message documentation #5956

Merged
merged 2 commits into from
Mar 24, 2022

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Mar 24, 2022

  • Add yourself to CONTRIBUTORS.txt if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
✨ New feature
🔨 Refactoring
📜 Docs

Description

Well, I immediately spotted a mistake...

We could also think about moving the message of empty-docstring to the doc_node attribute of the definition instead of on the definition line but that's for another PR.

Code is based on the functional test runner but I have removed everything that was redundant. I couldn't inherit the class because there was a lot of stuff to handle the .rc and .txt files. This seemed like a simpler approach.

I did not put this in the normal test suite as I didn't want to unnecessarily inflate local test runs. This is really something that only should be run before merging so the CI seemed a better fit.

@DanielNoord DanielNoord added Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow labels Mar 24, 2022
@DanielNoord DanielNoord added this to the 2.13.0 milestone Mar 24, 2022
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Looks great already ! I think reusing code would make it easier to maintain.

TESTS_NAMES = [f"{t[0]}-{t[1].stem}" for t in TESTS]


class LintModuleTest:
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, as that automatically disables some messages (that we might want to test) and can only accept FunctionalTestFile as test_file while we (I think) only want to pass a simple tuple.

MessageCounter = CounterType[Tuple[int, str]]


def get_functional_test_files_from_directory(input_dir: Path) -> List[Tuple[str, Path]]:
Copy link
Member

Choose a reason for hiding this comment

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

We could use https://github.com/PyCQA/pylint/blob/main/pylint/testutils/functional/find_functional_tests.py#L31, right ? ( yield from get_functional_test_files_from_directory if the file is good.py or bad.py ? Maybe we don't even need to check that it's good.py or bad.py)

Copy link
Collaborator Author

@DanielNoord DanielNoord Mar 24, 2022

Choose a reason for hiding this comment

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

No, that yields FunctionalTestFile:
https://github.com/PyCQA/pylint/blob/80838744c6f739fbf2f49bd1bfb70c98a84531aa/pylint/testutils/functional/test_file.py#L45

That class has a lot of additional methods that do not work with the folder structure of the messages documentation. Stuff like self._parse_options().
For the purpose of these tests we only need a tuple of test name and path so I don't think it makes much sense to try and change FunctionalTestFile to also allow a completely different directory structure.

doc/data/messages/e/empty-docstring/bad.py Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas merged commit 2518026 into pylint-dev:main Mar 24, 2022
@DanielNoord DanielNoord deleted the messages-documentation branch March 24, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants