-
Notifications
You must be signed in to change notification settings - Fork 909
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
Fix --tools=all
not working properly
#3361
Changes from 16 commits
5faebf3
0f61dd5
7e7ca3f
16ef432
87e2e1e
9ab8759
f127b48
4090586
47c4118
70985cc
a3eea61
fd7b734
6888463
143b204
ba0d696
dab4d29
d6672a3
f9802c7
b353f63
545755a
4bc1748
93d5b50
c875b3c
3df74ab
a821ede
d33b745
a228843
31d7a3f
3e6666e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1324,3 +1324,92 @@ def test_invalid_names(self, fake_kedro_cli, name): | |
"Kedro project names must contain only alphanumeric symbols, spaces, underscores and hyphens and be at least 2 characters long" | ||
in result.output | ||
) | ||
|
||
|
||
class TestParseYesNoToBools: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we move the tests for |
||
@pytest.mark.parametrize( | ||
"input", | ||
["yes", "YES", "y", "Y", "yEs"], | ||
) | ||
def parse_yes_no_to_bool_responds_true(self, input): | ||
assert _parse_yes_no_to_bool(input) is True | ||
|
||
@pytest.mark.parametrize( | ||
"input", | ||
["no", "NO", "n", "N", "No", ""], | ||
) | ||
def parse_yes_no_to_bool_responds_false(self, input): | ||
assert _parse_yes_no_to_bool(input) is False | ||
|
||
def parse_yes_no_to_bool_responds_none(self): | ||
assert _parse_yes_no_to_bool(None) is None | ||
|
||
|
||
class TestValidateSelection: | ||
def test_validate_selection_valid(self): | ||
tools = ["1", "2", "3", "4"] | ||
assert _validate_selection(tools) is None | ||
|
||
def test_validate_selection_invalid_single_tool(self): | ||
tools = ["8"] | ||
with pytest.raises(SystemExit): | ||
_validate_selection(tools) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add an assert for the correct error message? E.g the implementation for |
||
|
||
def test_validate_selection_invalid_multiple_tools(self): | ||
tools = ["8", "10", "15"] | ||
with pytest.raises(SystemExit): | ||
_validate_selection(tools) | ||
|
||
def test_validate_selection_mix_valid_invalid_tools(self): | ||
tools = ["1", "8", "3", "15"] | ||
with pytest.raises(SystemExit): | ||
_validate_selection(tools) | ||
|
||
def test_validate_selection_empty_list(self): | ||
tools = [] | ||
assert _validate_selection(tools) is None | ||
|
||
|
||
class TestConvertToolNamesToNumbers: | ||
def test_convert_tool_names_to_numbers_with_valid_tools(self): | ||
selected_tools = "lint,test,docs" | ||
result = _convert_tool_names_to_numbers(selected_tools) | ||
assert result == "1,2,4" | ||
|
||
def test_convert_tool_names_to_numbers_with_none(self): | ||
result = _convert_tool_names_to_numbers(None) | ||
assert result is None | ||
|
||
def test_convert_tool_names_to_numbers_with_empty_string(self): | ||
selected_tools = "" | ||
result = _convert_tool_names_to_numbers(selected_tools) | ||
assert result == "" | ||
|
||
def test_convert_tool_names_to_numbers_with_none_string(self): | ||
selected_tools = "none" | ||
result = _convert_tool_names_to_numbers(selected_tools) | ||
assert result == "" | ||
|
||
def test_convert_tool_names_to_numbers_with_all_string(self): | ||
result = _convert_tool_names_to_numbers("all") | ||
assert result == "1,2,3,4,5,6,7" | ||
|
||
def test_convert_tool_names_to_numbers_with_mixed_valid_invalid_tools(self): | ||
selected_tools = "lint,invalid_tool,docs" | ||
result = _convert_tool_names_to_numbers(selected_tools) | ||
assert result == "1,4" | ||
|
||
def test_convert_tool_names_to_numbers_with_whitespace(self): | ||
selected_tools = " lint , test , docs " | ||
result = _convert_tool_names_to_numbers(selected_tools) | ||
assert result == "1,2,4" | ||
|
||
def test_convert_tool_names_to_numbers_with_case_insensitive_tools(self): | ||
selected_tools = "Lint,TEST,Docs" | ||
result = _convert_tool_names_to_numbers(selected_tools) | ||
assert result == "1,2,4" | ||
|
||
def test_convert_tool_names_to_numbers_with_invalid_tools(self): | ||
selected_tools = "invalid_tool1,invalid_tool2" | ||
result = _convert_tool_names_to_numbers(selected_tools) | ||
assert result == "" |
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.
Maybe we just need to update the TOOLS_SHORTNAME_TO_NUMBER dictionary? Put "none" and "all" there? Or it will break something else?
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.
I was thinking this as well but it feels a bit weird as "all" and "none" aren't tools.