Skip to content

Add primer tests #78

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 8 commits into from
Mar 23, 2022
Merged

Add primer tests #78

merged 8 commits into from
Mar 23, 2022

Conversation

DanielNoord
Copy link
Owner

@DanielNoord DanielNoord commented Mar 17, 2022

TODO List:

  • Create branch with changes in PR and branch with merge-base (see mypy)
  • Run pydocstringformatter with merge-base branch in write mode
  • Checkout to changes branch
  • Do a non-write branch to get diff
  • Prettify diff
  • Probably tons more...

@DanielNoord DanielNoord marked this pull request as draft March 17, 2022 11:05
@coveralls
Copy link

coveralls commented Mar 17, 2022

Pull Request Test Coverage Report for Build 1999456191

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 1998301260: 0.0%
Covered Lines: 405
Relevant Lines: 405

💛 - Coveralls

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@DanielNoord
Copy link
Owner Author

@Pierre-Sassoulas Small achievement! It works. There are changes on main that would lead to changes to pylint, but the new version of the primer only shows the changes that would be introduced by this PR.

I'm going to do some code cleanup and add some annotations/comments but I actually think this is almost mergeable in an alpha form for the project.

It also gave me some good insight into how to develop this in the future for pylint.

@Pierre-Sassoulas
Copy link
Collaborator

It also gave me some good insight into how to develop this in the future for pylint.

Yeap, having that in pylint will make the new checkers and every modification SO MUCH BETTER. I'm super hyped in anticipation.

@DanielNoord
Copy link
Owner Author

Yeap, having that in pylint will make the new checkers and every modification SO MUCH BETTER. I'm super hyped in anticipation.

I have written most of the code here to be somewhat copyable to pylint as well. One of the big issues is the speed: we need to run the program twice for every package we "prime" one. For pydocstringformatter this isn't an issue. For pylint this is...
I was thinking about maybe creating a "needs-primer" label so we can only schedule primers when necessary.. Hm, first 2.13 though 😄

@Pierre-Sassoulas
Copy link
Collaborator

need to run the program twice for every package we "prime" one.

Unless we save the baseline in the repository ? We'd also need to specify the commit for each primer repository. OR maybe it's possible to make a job manual (I did not check the doc of github actions) ?

@DanielNoord
Copy link
Owner Author

Unless we save the baseline in the repository ? We'd also need to specify the commit for each primer repository. OR maybe it's possible to make a job manual (I did not check the doc of github actions) ?

Yeah, I was thinking about that as well. But I foresee issues with fixing false-positives and comparing those etc. There is probably a reason why mypy doesn't save the baseline and runs twice for every primer test. They have more than enough knowledgeable contributors to do that optimisation if they saw a possibility/necessity to do so.

Yeah we can. We might also want to start with a smaller subset of repositories to do false positive checking for. I know that home-assistant have actually disabled pylint in some of their CI because it takes too long. So us running over them is perhaps not the best choice.

@DanielNoord DanielNoord added this to the 0.6.0 milestone Mar 17, 2022
@DanielNoord DanielNoord added the primer Issues and PRs related to the primer test label Mar 17, 2022
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@DanielNoord DanielNoord marked this pull request as ready for review March 17, 2022 16:12
@github-actions
Copy link

According to the primer, this change has no effect on the checked open source code. 🤖🎉

@DanielNoord
Copy link
Owner Author

@Pierre-Sassoulas Not sure if you have the bandwidth to check this. I can merge as is, but I left it open to give you the opportunity to review (and potentially spot any errors that could be copied to pylint when I eventually copy this).

Copy link
Collaborator

@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.

To be fair I skimmed most of the code but LGTM. It looks great actually.

Comment on lines +54 to +61
You can also run the primer locally with minimal set-up. First you will need to make
a duplicate of your ``pydocstringformatter`` directory in a new ``./program_to_test``
directory. This is required so that the primer can check out multiple versions of the
program and compare the difference.

The next step is to follow the bash script in the ``Run primer`` step in the ``primer``
workflow file, found at ``.github/workflows/primer.yaml``. This should allow you to run
all necessary steps locally.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could create a test with pytest for that ? It seem automatable even if not very convenient because you need to duplicate the git repository.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, I think it does not offer much: when would the test pass or fail?
At some point I could explore trying to create a script that does the same and handles cloning the repository, but for now I think this is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ha right it makes no sense as a test. A script to set it up is more like it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
primer Issues and PRs related to the primer test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants