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

Feature/5 collect only actual test matter #9

Conversation

MisterRios
Copy link
Contributor

@MisterRios MisterRios commented Jul 2, 2018

Hi,

Thanks for the tip on finding the parsing string. From there, the problem just unravelled.

I have very mixed feelings on folder discovery. I'm not at all sure it is practical if the module finds all the test files. Is there a particular philosophy on it?

Also, is there any chance to add vim stuff to .gitignore? The swap files keep trying to get committed.
Thanks.

@anapaulagomes
Copy link
Owner

@anapaulagomes anapaulagomes commented Jul 4, 2018

I have very mixed feelings on folder discovery. I'm not at all sure it is practical if the module finds all the test files. Is there a particular philosophy on it?

I'm not sure if I get it. In this case, we are removing the tests that were collected already. So, we need to know if the collected tests are inside of that folder or not. I opened a PR few days ago to change this approach, since it was taking longer than needed. In this PR we are indicating the folders and files that pytest should collect (and execute). See #8.

Also, is there any chance to add vim stuff to .gitignore? The swap files keep trying to get committed.

I'm afraid not. Since this is from specifically from your development environment, I would advise you adding it to a global .gitignore.

Thanks for contributing!

Copy link
Owner

@anapaulagomes anapaulagomes left a comment

That's an essential part of this plugin. Thanks for bringing it!

pytest_picked.py Outdated
@@ -18,7 +21,8 @@ def pytest_collection_modifyitems(items, config):
if not picked_plugin:
return

picked_files, picked_folders = _affected_tests()
test_files = config._getini('python_files')
Copy link
Owner

@anapaulagomes anapaulagomes Jul 4, 2018

Choose a reason for hiding this comment

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

Maybe we should make it more implicit like test_file_convention. When I read test_files sounds like the tests files instead of the convention adopted.

Copy link
Contributor Author

@MisterRios MisterRios Jul 4, 2018

Choose a reason for hiding this comment

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

agreed. changed.

pytest_picked.py Outdated
@@ -64,15 +68,20 @@ def _affected_tests():
"""
raw_output = _get_git_status()

re_list = [item.replace('.', '\.').replace('*', '.*')
Copy link
Owner

@anapaulagomes anapaulagomes Jul 4, 2018

Choose a reason for hiding this comment

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

Since we're using Black to enforce the code style, you should use double quotes in this line. I added Lambda Lint to help us with it.

Copy link
Contributor Author

@MisterRios MisterRios Jul 4, 2018

Choose a reason for hiding this comment

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

also changed.

pytest_picked.py Outdated
if file_or_folder.endswith("/"):
if file_or_folder.endswith("/"):
path = Path(file_or_folder)
if any([part.startswith("test") for part in path.parts]):
Copy link
Owner

@anapaulagomes anapaulagomes Jul 4, 2018

Choose a reason for hiding this comment

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

In this case, I don't think we need to check if the folder has test on it. For instance, if you add a new folder called api and inside of it you have other folders or test_bla.py, we are going to miss it. git status will show just api/. IMO, for this case, we must rely on pytest to check the files.

Copy link
Contributor Author

@MisterRios MisterRios Jul 4, 2018

Choose a reason for hiding this comment

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

Couldn't we check inside the new folder for test files? This is what I meant by getting rid of folder checking and just checking the flles.
Where $ git status --short will return:

 M project/accounts/tests/test_api.py
?? newapp/

$ git status --short --untracked-files' will return:

 M project/accounts/tests/test_api.py
?? newapp/__init__.py
?? newapp/admin.py
?? newapp/apps.py
?? newapp/migrations/__init__.py
?? newapp/models.py
?? newapp/tests/test_views.py
?? newapp/views.py

So that we can just check by the file names. See: https://git-scm.com/docs/git-status/1.8.1#git-status---untracked-filesltmodegt

Oddly enough, whenever an untracked folder is added, git status immediately shows the individual files.

Copy link
Contributor Author

@MisterRios MisterRios Jul 5, 2018

Choose a reason for hiding this comment

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

Couldn't we check inside the new folder for test files? This is what I meant by getting rid of folder checking and just checking the flles.
Where $ git status --short will return:

 M project/accounts/tests/test_api.py
?? newapp/

$ git status --short --untracked-files' will return:

 M project/accounts/tests/test_api.py
?? newapp/__init__.py
?? newapp/admin.py
?? newapp/apps.py
?? newapp/migrations/__init__.py
?? newapp/models.py
?? newapp/tests/test_views.py
?? newapp/views.py

So that we can just check by the file names. see: https://git-scm.com/docs/git-status/1.8.1#git-status---untracked-filesltmodegt

Oddly enough, whenever an untracked folder is added, git status immediately shows the individual files.

Perhaps we address this in a separate ticket? For the meantime, I've changed it as requested.

@@ -139,11 +139,14 @@ def test_check_parser():
+ b" M picked.py\n"
+ b"A setup.py\n"
+ b" U tests/test_pytest_picked.py\n"
+ b"?? random/tests/"
+ b"?? random/tests/\n"
+ b" M intestine.py\n"
Copy link
Owner

@anapaulagomes anapaulagomes Jul 4, 2018

Choose a reason for hiding this comment

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

Nice! 👍

@MisterRios
Copy link
Contributor Author

@MisterRios MisterRios commented Jul 5, 2018

I've made the requested changes. Thanks again for explaining the project and answering all my questions. Let me know if you have any more feedback.

@anapaulagomes
Copy link
Owner

@anapaulagomes anapaulagomes commented Jul 7, 2018

I'm glad we're supporting pytest conventions. Thank you @MisterRios! 🥇
Once #8 is closed, I'll be happy to release a new version.

@anapaulagomes anapaulagomes merged commit a57d0c2 into anapaulagomes:master Jul 7, 2018
2 checks passed
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.

None yet

2 participants