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 tests for existing Pyreverse bugs #8983

Merged
merged 3 commits into from
Aug 31, 2023

Conversation

nickdrozd
Copy link
Contributor

Type of Changes

Type
🔨 Refactoring
📜 Docs

Description

Add tests for existing bugs:

Running the last of these tests successfully requires adding a sort call, which is an extremely minor change in behavior.

@nickdrozd nickdrozd added the Skip news 🔇 This change does not require a changelog entry label Aug 28, 2023
@Pierre-Sassoulas Pierre-Sassoulas added the pyreverse Related to pyreverse component label Aug 28, 2023
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.

LGTM

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #8983 (95b688a) into main (b487710) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8983   +/-   ##
=======================================
  Coverage   95.75%   95.75%           
=======================================
  Files         173      173           
  Lines       18604    18604           
=======================================
  Hits        17815    17815           
  Misses        789      789           
Files Changed Coverage Δ
pylint/pyreverse/diagrams.py 93.10% <100.00%> (ø)

@nickdrozd
Copy link
Contributor Author

nickdrozd commented Aug 29, 2023

BTW, I have fixes for all of these. The logical error is the same, but implemented in three different ways. Should they go up in one PR or in three separate PRs? Or just push straight to this one?

@Pierre-Sassoulas
Copy link
Member

Ha I thought you just wanted to add the tests. Well if you have the fixes how about creating 3 PRs and fixing at the same time as adding the relevant tests? Feels clearer if we need to check the git history / PR later imo.

@nickdrozd
Copy link
Contributor Author

The behaviors and changes involved are subtly different, so I think separating everything will help to make clear exactly what is affected by what. So if the tests for the buggy behavior are set up first, then the commits with the changes will show the changes to that behavior.

@nickdrozd
Copy link
Contributor Author

Added a test for #8671

Copy link
Collaborator

@DudeNr33 DudeNr33 left a comment

Choose a reason for hiding this comment

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

Sounds like a good plan to better understand the following PRs and changes in behaviour.

One heads up for the added sorted call:
#8776
We discussed if this is a wanted behavior or not, and ended with saying it would probably be best to add a command line switch to enable/disable sorting.
But that's a different topic, and as we already sort other stuff I'm totally fine with adding the sorted call here as well.

@DudeNr33 DudeNr33 merged commit 1427461 into pylint-dev:main Aug 31, 2023
42 checks passed
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.17.6 milestone Sep 4, 2023
github-actions bot pushed a commit that referenced this pull request Sep 4, 2023
* Add Pyreverse bug tests

* Sort Pyreverse class diagram names

* Add test for bad line break

(cherry picked from commit 1427461)
Pierre-Sassoulas pushed a commit that referenced this pull request Sep 4, 2023
* Add Pyreverse bug tests

* Sort Pyreverse class diagram names

* Add test for bad line break

(cherry picked from commit 1427461)

Co-authored-by: Nick Drozd <[email protected]>
@nickdrozd nickdrozd deleted the pyreverse-tests branch October 7, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported pyreverse Related to pyreverse component Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants