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 acceptance tests #1360

Merged
merged 1 commit into from
Jul 31, 2017
Merged

Conversation

rogalski
Copy link
Contributor

@rogalski rogalski commented Mar 2, 2017

It's a follow-up on discussion in #1072 comments.

One failing case on Py 3.6 found. One failing case on PyPy found.

Obviously these tests shouldn't run per check-in, but having them in source code and easily runnable may be beneficial.

Just an idea, we can think about it a little bit more.

Copy link
Contributor

@degustaf degustaf left a comment

Choose a reason for hiding this comment

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

Comparing the results of running this against @7df8caa, it appears that this added 48 tests. Meaning that it is scanning 48 modules. Looking at my system python, I am seeing that it should be closer to 197. Any idea why this is missing so many modules?

LIB_DIR = os.path.dirname(os.__file__)
MODULES_TO_CHECK = [f for f in os.listdir(LIB_DIR) if f.endswith(".py")
or os.path.exists(os.path.join(LIB_DIR, f, '__init__.py'))]
MODULES_TO_CHECK = MODULES_TO_CHECK
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for this line of code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@degustaf I had a slice there to limit module count during test runs when implementing it. Obviously it needs to be deleted.

@rogalski
Copy link
Contributor Author

rogalski commented Mar 3, 2017

I'll try to revisit module count somewhere around this weekend.
Feel free to cherry pick this change and work on it yourself if you want to.

Copy link
Contributor

@degustaf degustaf left a comment

Choose a reason for hiding this comment

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

@rogalski: Looking at travis, I see why you disabled all the messages to speed up the test suite. But, 3.6 had 3 failures, without finishing without disabling the actual checks. After this change, there is just one failure. We might need to reconsider what parts of the standard library to lint, or look into optimizing pylint to run faster.

@PCManticore
Copy link
Contributor

I think this is interesting, but I'm worried about the increase of the test running time, which tripled from what I'm seeing, and it does not provide too much value anyway, after running this the first time and fixing the found bugs. It would probably make more sense to have this under a flag, which we can enable before doing a release, and not with every commit. What do you think @rogalski and @degustaf ?

@degustaf
Copy link
Contributor

@PCManticore I agree that this only needs to be run periodically. I think having this as runnable when desired is a good idea. Perhaps move the acceptance directory out of test, and add documentation about running it using pytest.

@PCManticore
Copy link
Contributor

Hey @rogalski can you follow up on this? I think we just need to expose this under a flag, which we can run before every release.

@rogalski
Copy link
Contributor Author

Yeah, sure, although probably somewhere around next week.

I'd probably go for custom markers and reasonable default configuration on pytest, but feel free to suggest better solution.

@PCManticore
Copy link
Contributor

That sounds like a good idea @rogalski

@rogalski rogalski added Needs review 🔍 Needs to be reviewed by one or multiple more persons and removed Work in progress labels Jul 26, 2017
@rogalski
Copy link
Contributor Author

AppVeyor issue seems unrelated.

@PCManticore PCManticore merged commit 7d6114f into pylint-dev:master Jul 31, 2017
@PCManticore PCManticore removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jul 31, 2017
@rogalski rogalski deleted the test_acceptance branch August 11, 2017 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants