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

Update Testing Docs regarding Primer #7714

Closed
clavedeluna opened this issue Nov 5, 2022 · 11 comments · Fixed by #7758
Closed

Update Testing Docs regarding Primer #7714

clavedeluna opened this issue Nov 5, 2022 · 11 comments · Fixed by #7758
Labels
Documentation 📗 Needs PR This issue is accepted, sufficiently specified and now needs an implementation primer
Milestone

Comments

@clavedeluna
Copy link
Collaborator

clavedeluna commented Nov 5, 2022

Bug description

Primer testing docs mention batch_one and batch_two:

Screen Shot 2022-11-05 at 9 27 00 AM

at the very least, the suggested command with batch_two doesn't run any tests.

but I don't find any batch_two in the repo. Should this documentation be updated?

Configuration

No response

Command used

n/a

Pylint output

n/a

Expected behavior

n/a

Pylint version

n/a

OS / Environment

n/a

Additional dependencies

No response

@clavedeluna clavedeluna added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Nov 5, 2022
@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Nov 5, 2022

Yes, this is very out of date. Not only is there no longer a batch two, we aren't even running batch one anymore on CI the batch one packages are only checked for crashes, they don't get their messages diffed. The new primer that diffs messages is in tests/primer/__main__.py

@jacobtylerwalls jacobtylerwalls added Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Nov 5, 2022
@jacobtylerwalls jacobtylerwalls added this to the 2.16.0 milestone Nov 5, 2022
@mbyrnepr2
Copy link
Member

mbyrnepr2 commented Nov 5, 2022

@clavedeluna
Copy link
Collaborator Author

Ah! No I think I actually made a mistake. The link I provided
https://pylint.pycqa.org/en/v2.12.2/development_guide/testing.html#primer-tests
is clearly for an old version. Google just took me to that and I didn't notice.

Given https://pylint.pycqa.org/en/v2.15.5/development_guide/contributor_guide/tests/launching_test.html exists and has the correct docs, I'll close this issue.

Thanks for clarifying!

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.16.0 milestone Nov 5, 2022
@jacobtylerwalls
Copy link
Member

That documentation is still out of date. It only refers to the old primer.

@clavedeluna
Copy link
Collaborator Author

See #7685 (comment) for how to run primer

@mbyrnepr2
Copy link
Member

Would it be an idea to add the primer commands to tox and document how to run the primer using tox?

@Pierre-Sassoulas
Copy link
Member

That could work but primer are slow and tox also make things slower. (We'd have to check that the primed repoes are not downloaded by tox each time). I guess new contributor would appreciate to have a tox -e primer command and not having to read the full doc, and maintainer will know the faster command.

Regardless of what we do to make it easier to launch primer, we can close this issue once the primer are documented more clearly in the doc.

@clavedeluna
Copy link
Collaborator Author

I would be all for documenting the full commands

python tests/primer/__main__.py prepare --clone
python tests/primer/__main__.py run --type=pr

while also adding to tox and indicating both can be used.

As a frequent contributor, it took me waaay to long to figure out how to run primer locally...

@Pierre-Sassoulas
Copy link
Member

Also it should probably be launchable with a single command and no argument (python tests/primer/__main__.py).

@DanielNoord
Copy link
Collaborator

It was a conscious decision to separate the commands to be able to easily reuse currently cloned project.

We could add an full-run parser but good documentation would probably be more effective and easier.

@Pierre-Sassoulas
Copy link
Member

It was a conscious decision to separate the commands to be able to easily reuse currently cloned project.

Sure and it's a good thing, but we can auto-detect that the repositories were already cloned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Needs PR This issue is accepted, sufficiently specified and now needs an implementation primer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants