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 test for functional tests directories structure #5753

Merged
merged 2 commits into from
Feb 10, 2022

Conversation

DanielNoord
Copy link
Collaborator

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

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

Based on the discussion in #5474.

I'm not sure this is the best way to test this, but it ensures new contributors will run it as they are likely to run the pytest command at least once.

The test checks that a functional test should not be move to a sub-directory (use_test should go in u/use) and that it is not in the incorrect directory (using_test should not go in u/use).
I made two directories as the tests in those directories really do not follow this pattern and I don't think it makes too much sense to change all the tests names there.

@DanielNoord DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Jan 31, 2022
@DanielNoord DanielNoord added this to the 2.13.0 milestone Jan 31, 2022
@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Jan 31, 2022

Actually I had some code ready but I delayed pushing it as there was a misnamed symlink that was not very straightforward to rename. I modified the pylint/testutils/functional code. Let me open a MR with the current code so as to discuss this. (Edit : See #5757)

@DanielNoord
Copy link
Collaborator Author

For some reason this test passes locally... So I need to push to see what's wrong πŸ˜“

@DanielNoord
Copy link
Collaborator Author

So, this finally passes..

@coveralls
Copy link

coveralls commented Feb 1, 2022

Pull Request Test Coverage Report for Build 1826358503

  • 23 of 23 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 93.988%

Totals Coverage Status
Change from base Build 1826258595: 0.03%
Covered Lines: 14914
Relevant Lines: 15868

πŸ’› - Coveralls

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.

I think we could either:

or

But right now 5757 lacks a feature and there is a dissemination of structure check in this one. I think I prefer option 1 because it would permits to provide the structure to users of pylint/testutil.

@Pierre-Sassoulas
Copy link
Member

I rebased Daniel works into the two first commits then moved the test to pylint/testutils/ and took what was in the other MR, then renamed the symlink. Locally it was working, but I'm going to work on that / fix it tomorrow.

@cdce8p cdce8p removed their request for review February 1, 2022 23:07
@Pierre-Sassoulas
Copy link
Member

I'm still working on it :)

@DanielNoord
Copy link
Collaborator Author

@Pierre-Sassoulas Still working on this? Or want me to try and get this ready?

@Pierre-Sassoulas
Copy link
Member

I'm still working on it but could push the half-baked work in progress I have locally. There's an issue with moving some of the functional tests that have 48+ files in their directory. They are not entirely independents as they import each others so moving them break the tests. The easy fix is to change the imports, the real fix would be to make them independent. I often don't have a long enough uninterrupted period to do the real fix.

@DanielNoord
Copy link
Collaborator Author

I'm still working on it but could push the half-baked work in progress I have locally. There's an issue with moving some of the functional tests that have 48+ files in their directory. They are not entirely independents as they import each others so moving them break the tests. The easy fix is to change the imports, the real fix would be to make them independent. I often don't have a long enough uninterrupted period to do the real fix.

If you push the in progress work I'll take a look!

@Pierre-Sassoulas
Copy link
Member

I just did :) The 3 last commits are temporary garbage.

@DanielNoord DanielNoord force-pushed the functional-tests branch 2 times, most recently from 29193f5 to 64e8b2a Compare February 8, 2022 23:07
@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Feb 8, 2022

I think this is now good to go!

Edit: Coverage report is false. Those lines are already uncovered on main..

NOTE: PLEASE DON'T SQUASH.

We can use https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame to ignore the moving commit from git blame so that we don't lose easy access to relevant commits in git blame viewers (such as the one in VS Code).

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.

Sorry for the delay in reviewing. LGTM but a conflict appeared and we can't squash so we should rebase on latest main. Let me do it then.

@Pierre-Sassoulas Pierre-Sassoulas merged commit ea4560c into pylint-dev:main Feb 10, 2022
@DanielNoord DanielNoord deleted the functional-tests branch February 10, 2022 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants