-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
added feature only fetch the tests #112
Conversation
@anapaulagomes how locally I can test this PR to certify for CI? |
f7999ce
to
10f768b
Compare
Hi @omkarkhatavkar! Thanks for your contribution. Could you please add an entry to our CHANGELOG? PS.: this week I'm quite busy but I'll get back to your PR most likely next week. |
Hi @omkarkhatavkar! Just to let you know, I'll review your PR as soon as it has tests on it. This way we avoid back and forths. Thanks! |
@anapaulagomes got busy with Diwali Festival will be updating soon |
No worries, @omkarkhatavkar! Hope you had a good time. :) |
2b6f477
to
e1eefca
Compare
Yeah had nice time ! Now the tests are added. Let me know if there anything need to be added. |
@anapaulagomes I made some more logical change, added more validation with tests and supporting the test class as well. |
@anapaulagomes any update on this? |
Hi @omkarkhatavkar! I'll review it next week. Sorry for the delay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, @omkarkhatavkar! I'm really looking forward to having this feature merged. 👯 I left a few questions and raised some concerns.
@anapaulagomes there was a linting issue but not because of my PR also resolved that. |
Hi @omkarkhatavkar! Just so you know, I'll take a look on it next week. 🙏🏽 |
Any update on this @anapaulagomes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @omkarkhatavkar, sorry for not getting back to you when I promised. The truth is: I've been involved in different open source projects in my free time and I try to do my best in each of them. This means that it takes much of my free time. That said: I'll try to review the next changes as soon as I can - please don't expect it to be on the next day. Hope you understand this.
As I said before, I'm really looking forward to releasing this feature. But we still need to do some work here. I left a few comments on how the code can be improved. I also would like to suggest adding more tests for all different scenarios: test files, classes, and test methods. After breaking the method only_tests
down is going to be easier to write tests for each of them. Also, I think we should invest time in adding different diff files for these scenarios. This will bring the security we need that the feature is working fine and it's change proof.
Thanks for the work you've done! 🥇
@@ -53,7 +53,8 @@ def test_parser_should_ignore_no_paths_characteres(self, line, expected_line): | |||
assert parsed_line == expected_line | |||
|
|||
def test_should_list_unstaged_changed_files_as_affected_tests(self): | |||
test_file_convention = ["test_*.py", "*_test.py"] | |||
test_conventions = dict() | |||
test_conventions["file"] = ["test_*.py", "*_test.py"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can directly create the dict with the file values:
test_conventions["file"] = ["test_*.py", "*_test.py"] | |
test_conventions = { | |
"file": ["test_*.py", "*_test.py"] | |
} |
@@ -135,11 +136,12 @@ def test_should_list_branch_changed_files_as_affected_tests(self): | |||
b"tests/test_new_pytest_picked.py\n" | |||
b"M tests/test_other_module.py" | |||
) | |||
test_file_convention = ["test_*.py", "*_test.py"] | |||
test_conventions = dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
b"\n+++ b/tests/pytestpicked/test_modes6.py" | ||
b"\n@@ -191,29 +191,8 @@ class InvalidClass:" | ||
) | ||
test_conventions = dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
test_conventions = dict() | ||
test_conventions["file"] = ["test_*.py", "*_test.py"] | ||
test_conventions["class"] = ["Test", "Check"] | ||
test_conventions["function"] = ["test", "check_*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different conventions are passed but the input doesn't include all of them (tests with Check
in the beginning and *_test.py
are missing, for example).
"unstaged": Unstaged(test_file_convention), | ||
"branch": Branch(test_conventions), | ||
"unstaged": Unstaged(test_conventions), | ||
"onlychanged": OnlyChanged(test_conventions, only_modified_tests=True), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍🏽
if re.search(class_regex, file_or_test): | ||
match = re.findall(class_regex, file_or_test) | ||
class_name = match[0] | ||
file_dict[last_file_name].append(class_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file_dict[last_file_name].append(class_name) | |
file_dict[last_file_name].append(match[0]) |
class_name
won't be used anywhere again so we can append it directly.
if not was_it_class: | ||
match = re.findall(test_regex, file_or_test) | ||
test_name = match[0] | ||
file_dict[last_file_name].append(class_name + "::" + test_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file_dict[last_file_name].append(class_name + "::" + test_name) | |
file_dict[last_file_name].append(class_name + "::" + match[0]) |
|
||
elif re.search(test_regex, file_or_test): | ||
if not was_it_class: | ||
match = re.findall(test_regex, file_or_test) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I couldn't understand why the class validation is needed. Would care to explain it to me a bit more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you're doing double regex parsing again.
test_name = match[0] | ||
file_dict[last_file_name].append(class_name + "::" + test_name) | ||
class_name = "" | ||
for file_name in file_dict.keys(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for file_name in file_dict.keys(): | |
for file_name, test_names in file_dict.items(): | |
for test_name in test_names : |
b"\n+++ b/tests/pytestpicked/test_modes5.py" | ||
b"\n+ def invalid_test(self):" | ||
b"\n+++ b/tests/pytestpicked/test_modes6.py" | ||
b"\n@@ -191,29 +191,8 @@ class InvalidClass:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All lines are about the things we want to extract. But it isn't how real diffs looks like. So would be nice to have some real diff here. Feel free to add a file in a directory in our test folder if you think it would be better.
Closing stale PR. |
Fixed #93
New Command ?
pytest --picked --mode=onlychanged
How to use this ?
If you wanted to run modified or newly added tests here is the example of that
With this change or PR we can easily run those tests instead whole file