Better primer tests : more reliable, easier to understand, and more stable

[Pierre-Sassoulas]

Current problem

• New false positives can appear when a commit is added on one of the primed repository. we ignore ir when it's unrelated with the current MR then we never think about it again
• When the primer crash there's no message given in the output, false positive unnecessary-list-index-lookup for enumerate #7685 (comment)
• General lack of stability of primer due to caching or other issues
• When the content of the message change but the location and symbol/msgid is the same there's a new message and deleted message so it's harder to see what happened .

Desired solution

• Launch primer on fixed commit so we don't have unrelated change in every MR
• Upgrade commit periodically (before release ?) so we actually check the new messages (false positive ?) at some point.
• Better logging in case of crash with the traceback being correctly printed
• Maybe more unit tests to reduce the number of hard to debug issue
• Finish [primer] Create a class for easier comparison of pylint results #6984 to use a diff on modified message instead of having an addition and deletion
• Use default values and not our own pylintrc values for more stability and to see what standard user without configuration would have

[DanielNoord]

• New false positives can appear when a commit is added on one of the primed repository. we ignore ir when it's unrelated with the current MR then we never think about it again

I don't really understand this. The PR run should check that the commit it is running on is the same as on Main, so any new messages (if all works well) should come from the PR.
If this is indeed happening it seems like the check for commits it not good enough.

• When the primer crash there's no message given in the output, false positive unnecessary-list-index-lookup for enumerate #7685 (comment)

This probably warrants its own issue as it is easily fixable and self contained.

• General lack of stability of primer due to caching or other issues

We would need to do some more investigation as to why this happens.

• When the content of the message change but the location and symbol/msgid is the same there's a new message and deleted message so it's harder to see what happened .

We should probably ask contributors to split such PRs.

[Pierre-Sassoulas]

If this is indeed happening it seems like the check for commits it not good enough.

See #7725 (comment)

When the content of the message change but the location and symbol/msgid is the same there's a new message and deleted message so it's harder to see what happened .

We should probably ask contributors to split such PRs.

I mean in a PR like this one : #7723 , the primer says:

The following messages are now emitted:

Details

1. compare-to-zero:
   "open_brackets == 0" can be simplified to "not open_brackets" as 0 is falsey
   https://github.com/PyCQA/astroid/blob/4acf5785c54f4ccb8f41402991f6ddc5d8b28b89/astroid/rebuilder.py#L198

The following messages are no longer emitted:

Details

1. compare-to-zero:
   Avoid comparisons to zero
   https://github.com/PyCQA/astroid/blob/4acf5785c54f4ccb8f41402991f6ddc5d8b28b89/astroid/rebuilder.py#L190

But it could say:

The following messages content changed:

Details

1. compare-to-zero -> use-implicit-booleaness-not-comparison-to-zero:

+ *"open_brackets == 0" can be simplified to "not open_brackets" as 0 is falsey*
- *Avoid comparisons to zero*

https://github.com/PyCQA/astroid/blob/4acf5785c54f4ccb8f41402991f6ddc5d8b28b89/astroid/rebuilder.py#L190

This is what I had in mind with the primer refactor I started some time ago and did not have time to finish yet (and there's now conflicts 😄 )

[DanielNoord]

Ah that last change seems good, although again, probably deserves it's own issue as these "main tracking issues" never seem to go anywhere (there is a very similar issue about the primer just like this open still).

With respect to that PR and comment: we would need to investigate what's going on there. Both the Main and PR run report to be running over the same sentry commit. It's probably something to do with invalid caches or something, but I don't think we should assume it is due to "commits slipping in when doing the PR run": that still seems to work...

Edit: I thought of another potential issue: the pulling of main into the PR branch. That might not always go perfectly, it's called Pull 'main' in the action and might be worth investigating for suspect comments.

[Pierre-Sassoulas]

I like the checklists around a topic because we have 650+ issues and things get lost or duplicated easily and conversation happens all over the place (when and where a problem happen). So some centralization for subject with a lot of things to do like primer or python interpreters or releases helps imo. There's definitely going to be a separate (even multiple) pull request for each though. We can also open a new issue if you think one of the checklist item is contentious and need to be discussed.

[Pierre-Sassoulas]

The primer job is failing on all pipeline lately with the same message in the "check cache" step, any idea @DanielNoord @cdce8p ? https://github.com/PyCQA/pylint/actions/runs/3437842348/jobs/5733137039:

Traceback (most recent call last):
  File "/home/runner/work/pylint/pylint/tests/primer/__main__.py", line 17, in <module>
    primer.run()
  File "/home/runner/work/pylint/pylint/pylint/testutils/_primer/primer.py", line 93, in run
    self.command.run()
  File "/home/runner/work/pylint/pylint/pylint/testutils/_primer/primer_prepare_command.py", line 22, in run
    local_commit = Repo(data.clone_directory).head.object.hexsha
  File "/home/runner/work/pylint/pylint/venv/lib/python3.10/site-packages/git/repo/base.py", line 194, in __init__
    raise NoSuchPathError(epath)
git.exc.NoSuchPathError: /home/runner/work/pylint/pylint/tests/.pylint_primer_tests/PyCQA/astroid

[cdce8p]

The primer job is failing on all pipeline lately with the same message in the "check cache" step, any idea @DanielNoord @cdce8p ? https://github.com/PyCQA/pylint/actions/runs/3437842348/jobs/5733137039:

The project cache restore from main failed. Looking at the commit and action history, it seems like the failures started after music21 was moved in 0c2fe7f. Could you try if merging upstream/main in the PRs fixes the issue?

[Pierre-Sassoulas]

We should use pylint's default values and not pylint's own configuration during the primer to avoid this: #7796 (comment)