Feat/docs and tests: Add tests for utils and code coverage #10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR integrates coverage tooling and CI reporting, enhances utility functions with logging and robust error handling, refines task merging logic, and adds comprehensive unit tests for utility functions. Class diagram for updated utility functions in style_predictor.utilsclassDiagram
class group_openings_with_eco {
+list[tuple[str, str, int]] data
+dict[str, dict[str, Any]] return
}
class sort_openings {
+dict[str, dict[str, dict[str, int | list[str]]]] openings
+list return
}
class average_rating {
+dict[str, tuple[float, int]] data
+dict[str, float] return
+LOG: logging.Logger
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @CalvoM - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `style_predictor/tests/test_utils.py:43` </location>
<code_context>
+ ),
+ ],
+ )
+ def test_group_openings_with_eco(self, opening_data, expected):
+ assert group_openings_with_eco(opening_data) == expected # nosec
+
+ @pytest.mark.parametrize(
</code_context>
<issue_to_address>
Consider adding edge case tests for empty input and duplicate ECO codes.
Adding tests for these edge cases will help verify correct handling of empty input, duplicate ECO codes for the same opening, and identical ECO codes with different opening names.
Suggested implementation:
```python
[
# Normal case
(
(
("E1", "Opening_1", 1),
("E2", "Opening_2", 2),
("E3", "Opening_3", 3),
("E1", "Opening_1", 4),
),
{
```
```python
{
```
```python
),
# Edge case: empty input
(
(),
{},
),
# Edge case: duplicate ECO codes for the same opening
(
(
("E1", "Opening_1", 1),
("E1", "Opening_1", 2),
("E1", "Opening_1", 3),
),
{
"Opening_1": {"total": 6, "eco_codes": ["E1"]},
},
),
# Edge case: identical ECO codes with different opening names
(
(
("E1", "Opening_1", 1),
("E1", "Opening_2", 2),
),
{
"Opening_1": {"total": 1, "eco_codes": ["E1"]},
"Opening_2": {"total": 2, "eco_codes": ["E1"]},
},
),
```
</issue_to_address>
### Comment 2
<location> `style_predictor/tests/test_utils.py:75` </location>
<code_context>
+ ),
+ ],
+ )
+ def test_sort_openings(self, openings, expected):
+ assert sort_openings(openings) == expected # nosec
+
+ @pytest.mark.parametrize(
</code_context>
<issue_to_address>
Add tests for edge cases in sort_openings, such as ties and fewer than five openings.
Please add tests for fewer than five openings, ties in 'total', and empty input to ensure robustness.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_sort_openings(self, openings, expected):
assert sort_openings(openings) == expected # nosec
=======
def test_sort_openings(self, openings, expected):
assert sort_openings(openings) == expected # nosec
@pytest.mark.parametrize(
"openings,expected",
[
# Fewer than five openings
(
{
"Opening_1": {"total": 2, "eco_codes": ["E1"]},
"Opening_2": {"total": 1, "eco_codes": ["E2"]},
},
[
("Opening_1", {"total": 2, "eco_codes": ["E1"]}),
("Opening_2", {"total": 1, "eco_codes": ["E2"]}),
],
),
# Ties in 'total'
(
{
"Opening_1": {"total": 3, "eco_codes": ["E1"]},
"Opening_2": {"total": 3, "eco_codes": ["E2"]},
"Opening_3": {"total": 2, "eco_codes": ["E3"]},
},
[
("Opening_1", {"total": 3, "eco_codes": ["E1"]}),
("Opening_2", {"total": 3, "eco_codes": ["E2"]}),
("Opening_3", {"total": 2, "eco_codes": ["E3"]}),
],
),
# Empty input
(
{},
[],
),
],
)
def test_sort_openings_edge_cases(self, openings, expected):
assert sort_openings(openings) == expected # nosec
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `style_predictor/tests/test_utils.py:91` </location>
<code_context>
+ ),
+ ],
+ )
+ def test_average_rating(self, game_rating, expected):
+ assert average_rating(game_rating) == expected # nosec
+
+ @pytest.mark.parametrize(
</code_context>
<issue_to_address>
Add tests for missing or malformed keys in average_rating input.
Please add tests for empty input dicts, non-tuple values, tuples with incorrect lengths, and negative values to ensure robust input handling.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
(
{"hyper": (10, 2), "bullet": (15, 2), "classical": (20, 2)},
{"hyper": 5.0, "bullet": 7.5, "classical": 10.0},
),
(
{"hyper": (10, 0), "bullet": (0, 0), "classical": (20, 2)},
{"hyper": 0, "bullet": 0, "classical": 10.0},
),
],
)
=======
(
{"hyper": (10, 2), "bullet": (15, 2), "classical": (20, 2)},
{"hyper": 5.0, "bullet": 7.5, "classical": 10.0},
),
(
{"hyper": (10, 0), "bullet": (0, 0), "classical": (20, 2)},
{"hyper": 0, "bullet": 0, "classical": 10.0},
),
# Empty input dict
(
{},
{},
),
# Non-tuple value
(
{"hyper": 5},
{"hyper": 0},
),
# Tuple with incorrect length (1 element)
(
{"hyper": (10,)},
{"hyper": 0},
),
# Tuple with incorrect length (3 elements)
(
{"hyper": (10, 2, 3)},
{"hyper": 0},
),
# Negative values
(
{"hyper": (-10, 2)},
{"hyper": -5.0},
),
(
{"hyper": (10, -2)},
{"hyper": 0},
),
(
{"hyper": (-10, -2)},
{"hyper": 0},
),
],
)
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `style_predictor/tests/test_utils.py:115` </location>
<code_context>
+ ),
+ ],
+ )
+ def test_merge_game_objects(self, game_objects, expected):
+ res = {}
+ for obj in game_objects:
+ merge_game_objects(res, obj)
+ assert res == expected # nosec
+
+ @pytest.mark.parametrize(
</code_context>
<issue_to_address>
Consider adding tests for merge_game_objects with missing or extra keys.
Please add tests for cases with missing keys, extra keys, and an empty input list to ensure robustness against varying input shapes.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
(
[
{"count": 1, "wins": 2, "losses": 3},
{"count": 1, "wins": 2, "losses": 3},
{"count": 1, "wins": 2, "losses": 3},
],
{"count": 3, "wins": 6, "losses": 9},
),
],
)
=======
(
[
{"count": 1, "wins": 2, "losses": 3},
{"count": 1, "wins": 2, "losses": 3},
{"count": 1, "wins": 2, "losses": 3},
],
{"count": 3, "wins": 6, "losses": 9},
),
# Test with missing keys in some objects
(
[
{"count": 1, "wins": 2},
{"count": 1, "losses": 3},
{"wins": 2, "losses": 3},
],
{"count": 2, "wins": 4, "losses": 6},
),
# Test with extra keys in some objects
(
[
{"count": 1, "wins": 2, "losses": 3, "draws": 1},
{"count": 1, "wins": 2, "losses": 3},
{"count": 1, "wins": 2, "losses": 3, "draws": 2},
],
{"count": 3, "wins": 6, "losses": 9, "draws": 3},
),
# Test with empty input list
(
[],
{},
),
# Test with all objects missing all keys (empty dicts)
(
[{}, {}, {}],
{},
),
],
)
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+75
to
+128
| def test_sort_openings(self, openings, expected): | ||
| assert sort_openings(openings) == expected # nosec |
There was a problem hiding this comment.
suggestion (testing): Add tests for edge cases in sort_openings, such as ties and fewer than five openings.
Please add tests for fewer than five openings, ties in 'total', and empty input to ensure robustness.
Suggested change
| def test_sort_openings(self, openings, expected): | |
| assert sort_openings(openings) == expected # nosec | |
| def test_sort_openings(self, openings, expected): | |
| assert sort_openings(openings) == expected # nosec | |
| @pytest.mark.parametrize( | |
| "openings,expected", | |
| [ | |
| # Fewer than five openings | |
| ( | |
| { | |
| "Opening_1": {"total": 2, "eco_codes": ["E1"]}, | |
| "Opening_2": {"total": 1, "eco_codes": ["E2"]}, | |
| }, | |
| [ | |
| ("Opening_1", {"total": 2, "eco_codes": ["E1"]}), | |
| ("Opening_2", {"total": 1, "eco_codes": ["E2"]}), | |
| ], | |
| ), | |
| # Ties in 'total' | |
| ( | |
| { | |
| "Opening_1": {"total": 3, "eco_codes": ["E1"]}, | |
| "Opening_2": {"total": 3, "eco_codes": ["E2"]}, | |
| "Opening_3": {"total": 2, "eco_codes": ["E3"]}, | |
| }, | |
| [ | |
| ("Opening_1", {"total": 3, "eco_codes": ["E1"]}), | |
| ("Opening_2", {"total": 3, "eco_codes": ["E2"]}), | |
| ("Opening_3", {"total": 2, "eco_codes": ["E3"]}), | |
| ], | |
| ), | |
| # Empty input | |
| ( | |
| {}, | |
| [], | |
| ), | |
| ], | |
| ) | |
| def test_sort_openings_edge_cases(self, openings, expected): | |
| assert sort_openings(openings) == expected # nosec |
Comment on lines
81
to
169
| ( | ||
| {"hyper": (10, 2), "bullet": (15, 2), "classical": (20, 2)}, | ||
| {"hyper": 5.0, "bullet": 7.5, "classical": 10.0}, | ||
| ), | ||
| ( | ||
| {"hyper": (10, 0), "bullet": (0, 0), "classical": (20, 2)}, | ||
| {"hyper": 0, "bullet": 0, "classical": 10.0}, | ||
| ), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
suggestion (testing): Add tests for missing or malformed keys in average_rating input.
Please add tests for empty input dicts, non-tuple values, tuples with incorrect lengths, and negative values to ensure robust input handling.
Suggested change
| ( | |
| {"hyper": (10, 2), "bullet": (15, 2), "classical": (20, 2)}, | |
| {"hyper": 5.0, "bullet": 7.5, "classical": 10.0}, | |
| ), | |
| ( | |
| {"hyper": (10, 0), "bullet": (0, 0), "classical": (20, 2)}, | |
| {"hyper": 0, "bullet": 0, "classical": 10.0}, | |
| ), | |
| ], | |
| ) | |
| ( | |
| {"hyper": (10, 2), "bullet": (15, 2), "classical": (20, 2)}, | |
| {"hyper": 5.0, "bullet": 7.5, "classical": 10.0}, | |
| ), | |
| ( | |
| {"hyper": (10, 0), "bullet": (0, 0), "classical": (20, 2)}, | |
| {"hyper": 0, "bullet": 0, "classical": 10.0}, | |
| ), | |
| # Empty input dict | |
| ( | |
| {}, | |
| {}, | |
| ), | |
| # Non-tuple value | |
| ( | |
| {"hyper": 5}, | |
| {"hyper": 0}, | |
| ), | |
| # Tuple with incorrect length (1 element) | |
| ( | |
| {"hyper": (10,)}, | |
| {"hyper": 0}, | |
| ), | |
| # Tuple with incorrect length (3 elements) | |
| ( | |
| {"hyper": (10, 2, 3)}, | |
| {"hyper": 0}, | |
| ), | |
| # Negative values | |
| ( | |
| {"hyper": (-10, 2)}, | |
| {"hyper": -5.0}, | |
| ), | |
| ( | |
| {"hyper": (10, -2)}, | |
| {"hyper": 0}, | |
| ), | |
| ( | |
| {"hyper": (-10, -2)}, | |
| {"hyper": 0}, | |
| ), | |
| ], | |
| ) |
Comment on lines
105
to
207
| ( | ||
| [ | ||
| {"count": 1, "wins": 2, "losses": 3}, | ||
| {"count": 1, "wins": 2, "losses": 3}, | ||
| {"count": 1, "wins": 2, "losses": 3}, | ||
| ], | ||
| {"count": 3, "wins": 6, "losses": 9}, | ||
| ), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for merge_game_objects with missing or extra keys.
Please add tests for cases with missing keys, extra keys, and an empty input list to ensure robustness against varying input shapes.
Suggested change
| ( | |
| [ | |
| {"count": 1, "wins": 2, "losses": 3}, | |
| {"count": 1, "wins": 2, "losses": 3}, | |
| {"count": 1, "wins": 2, "losses": 3}, | |
| ], | |
| {"count": 3, "wins": 6, "losses": 9}, | |
| ), | |
| ], | |
| ) | |
| ( | |
| [ | |
| {"count": 1, "wins": 2, "losses": 3}, | |
| {"count": 1, "wins": 2, "losses": 3}, | |
| {"count": 1, "wins": 2, "losses": 3}, | |
| ], | |
| {"count": 3, "wins": 6, "losses": 9}, | |
| ), | |
| # Test with missing keys in some objects | |
| ( | |
| [ | |
| {"count": 1, "wins": 2}, | |
| {"count": 1, "losses": 3}, | |
| {"wins": 2, "losses": 3}, | |
| ], | |
| {"count": 2, "wins": 4, "losses": 6}, | |
| ), | |
| # Test with extra keys in some objects | |
| ( | |
| [ | |
| {"count": 1, "wins": 2, "losses": 3, "draws": 1}, | |
| {"count": 1, "wins": 2, "losses": 3}, | |
| {"count": 1, "wins": 2, "losses": 3, "draws": 2}, | |
| ], | |
| {"count": 3, "wins": 6, "losses": 9, "draws": 3}, | |
| ), | |
| # Test with empty input list | |
| ( | |
| [], | |
| {}, | |
| ), | |
| # Test with all objects missing all keys (empty dicts) | |
| ( | |
| [{}, {}, {}], | |
| {}, | |
| ), | |
| ], | |
| ) |
style_predictor/tests/test_utils.py
Outdated
Comment on lines
117
to
206
| for obj in game_objects: | ||
| merge_game_objects(res, obj) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
style_predictor/tests/test_utils.py
Outdated
Comment on lines
268
to
357
| for obj in game_objects: | ||
| res = merge_game_objects(res, obj) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
CalvoM
force-pushed
the
feat/docs_and_tests
branch
3 times, most recently
from
2bcff2f to
9935fce
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @CalvoM - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `style_predictor/utils.py:28` </location>
<code_context>
-def sort_openings(openings):
+def sort_openings(openings: dict[str, dict[str, dict[str, int | list[str]]]]):
return sorted(openings.items(), key=lambda item: item[1]["total"], reverse=True)[:5]
</code_context>
<issue_to_address>
The type annotation for 'openings' in sort_openings is overly complex and may not match actual usage.
Consider simplifying the type annotation or using a TypedDict if only specific keys or value types are needed, to improve clarity and maintainability.
Suggested implementation:
```python
from typing import TypedDict
LOG = logging.getLogger(__name__)
```
```python
class OpeningInfo(TypedDict):
total: int
# Add other fields as needed, e.g.:
# moves: list[str]
def sort_openings(openings: dict[str, OpeningInfo]):
return sorted(openings.items(), key=lambda item: item[1]["total"], reverse=True)[:5]
```
</issue_to_address>
### Comment 2
<location> `style_predictor/tests/test_utils.py:203` </location>
<code_context>
+ ),
+ ],
+ )
+ def test_merge_game_objects(self, game_objects, expected):
+ res = {}
+ for obj in game_objects:
+ merge_game_objects(res, obj)
+ assert res == expected # nosec
+
+ @pytest.mark.parametrize(
</code_context>
<issue_to_address>
Add test for merge_game_objects with conflicting key types.
Please add a test case where the same key has different types across objects to ensure merge_game_objects handles or reports type conflicts correctly.
Suggested implementation:
```python
# Test with all objects missing all keys (empty dicts)
(
[{}, {}, {}],
{},
),
# Test with conflicting key types
(
[{"a": 1}, {"a": [2, 3]}],
pytest.raises(TypeError),
),
],
)
```
```python
res = {}
if isinstance(expected, type) and issubclass(expected, Exception):
with expected:
for obj in game_objects:
merge_game_objects(res, obj)
else:
for obj in game_objects:
merge_game_objects(res, obj)
assert res == expected # nosec
```
</issue_to_address>
### Comment 3
<location> `style_predictor/tests/test_utils.py:109` </location>
<code_context>
+ ),
+ ],
+ )
+ def test_sort_openings(self, openings, expected):
+ assert sort_openings(openings) == expected # nosec
+
+ @pytest.mark.parametrize(
</code_context>
<issue_to_address>
Consider testing sort_openings with more than five openings.
Adding such a test will verify that the function correctly limits the output to five and maintains the correct sort order.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
("Opening_1", {"total": 3, "eco_codes": ["E1"]}),
("Opening_2", {"total": 3, "eco_codes": ["E2"]}),
("Opening_3", {"total": 2, "eco_codes": ["E3"]}),
],
),
],
)
=======
("Opening_1", {"total": 3, "eco_codes": ["E1"]}),
("Opening_2", {"total": 3, "eco_codes": ["E2"]}),
("Opening_3", {"total": 2, "eco_codes": ["E3"]}),
],
),
# New test: more than five openings, should return only top five sorted
(
{
"Opening_A": {"total": 10, "eco_codes": ["A"]},
"Opening_B": {"total": 8, "eco_codes": ["B"]},
"Opening_C": {"total": 15, "eco_codes": ["C"]},
"Opening_D": {"total": 5, "eco_codes": ["D"]},
"Opening_E": {"total": 12, "eco_codes": ["E"]},
"Opening_F": {"total": 7, "eco_codes": ["F"]},
"Opening_G": {"total": 9, "eco_codes": ["G"]},
},
[
("Opening_C", {"total": 15, "eco_codes": ["C"]}),
("Opening_E", {"total": 12, "eco_codes": ["E"]}),
("Opening_A", {"total": 10, "eco_codes": ["A"]}),
("Opening_G", {"total": 9, "eco_codes": ["G"]}),
("Opening_B", {"total": 8, "eco_codes": ["B"]}),
],
),
],
)
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
|
|
||
| def sort_openings(openings): | ||
| def sort_openings(openings: dict[str, dict[str, dict[str, int | list[str]]]]): |
There was a problem hiding this comment.
suggestion: The type annotation for 'openings' in sort_openings is overly complex and may not match actual usage.
Consider simplifying the type annotation or using a TypedDict if only specific keys or value types are needed, to improve clarity and maintainability.
Suggested implementation:
from typing import TypedDict
LOG = logging.getLogger(__name__)
class OpeningInfo(TypedDict):
total: int
# Add other fields as needed, e.g.:
# moves: list[str]
def sort_openings(openings: dict[str, OpeningInfo]):
return sorted(openings.items(), key=lambda item: item[1]["total"], reverse=True)[:5]
Comment on lines
203
to
211
| def test_merge_game_objects(self, game_objects, expected): | ||
| res = {} | ||
| for obj in game_objects: | ||
| merge_game_objects(res, obj) | ||
| assert res == expected # nosec |
There was a problem hiding this comment.
suggestion (testing): Add test for merge_game_objects with conflicting key types.
Please add a test case where the same key has different types across objects to ensure merge_game_objects handles or reports type conflicts correctly.
Suggested implementation:
# Test with all objects missing all keys (empty dicts)
(
[{}, {}, {}],
{},
),
# Test with conflicting key types
(
[{"a": 1}, {"a": [2, 3]}],
pytest.raises(TypeError),
),
],
) res = {}
if isinstance(expected, type) and issubclass(expected, Exception):
with expected:
for obj in game_objects:
merge_game_objects(res, obj)
else:
for obj in game_objects:
merge_game_objects(res, obj)
assert res == expected # nosec
Comment on lines
102
to
126
| ("Opening_1", {"total": 3, "eco_codes": ["E1"]}), | ||
| ("Opening_2", {"total": 3, "eco_codes": ["E2"]}), | ||
| ("Opening_3", {"total": 2, "eco_codes": ["E3"]}), | ||
| ], | ||
| ), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
suggestion (testing): Consider testing sort_openings with more than five openings.
Adding such a test will verify that the function correctly limits the output to five and maintains the correct sort order.
Suggested change
| ("Opening_1", {"total": 3, "eco_codes": ["E1"]}), | |
| ("Opening_2", {"total": 3, "eco_codes": ["E2"]}), | |
| ("Opening_3", {"total": 2, "eco_codes": ["E3"]}), | |
| ], | |
| ), | |
| ], | |
| ) | |
| ("Opening_1", {"total": 3, "eco_codes": ["E1"]}), | |
| ("Opening_2", {"total": 3, "eco_codes": ["E2"]}), | |
| ("Opening_3", {"total": 2, "eco_codes": ["E3"]}), | |
| ], | |
| ), | |
| # New test: more than five openings, should return only top five sorted | |
| ( | |
| { | |
| "Opening_A": {"total": 10, "eco_codes": ["A"]}, | |
| "Opening_B": {"total": 8, "eco_codes": ["B"]}, | |
| "Opening_C": {"total": 15, "eco_codes": ["C"]}, | |
| "Opening_D": {"total": 5, "eco_codes": ["D"]}, | |
| "Opening_E": {"total": 12, "eco_codes": ["E"]}, | |
| "Opening_F": {"total": 7, "eco_codes": ["F"]}, | |
| "Opening_G": {"total": 9, "eco_codes": ["G"]}, | |
| }, | |
| [ | |
| ("Opening_C", {"total": 15, "eco_codes": ["C"]}), | |
| ("Opening_E", {"total": 12, "eco_codes": ["E"]}), | |
| ("Opening_A", {"total": 10, "eco_codes": ["A"]}), | |
| ("Opening_G", {"total": 9, "eco_codes": ["G"]}), | |
| ("Opening_B", {"total": 8, "eco_codes": ["B"]}), | |
| ], | |
| ), | |
| ], | |
| ) |
style_predictor/tests/test_utils.py
Outdated
Comment on lines
205
to
206
| for obj in game_objects: | ||
| merge_game_objects(res, obj) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
CalvoM
force-pushed
the
feat/docs_and_tests
branch
from
9935fce to
6709532
Compare
CalvoM
force-pushed
the
feat/docs_and_tests
branch
from
6709532 to
9c40865
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Integrate code coverage tooling and bolster utility function robustness with logging and error handling while adding end-to-end tests and CI coverage reporting.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests: