ux: Better assignment tests UX #384
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @yammesicka - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| .code-review-container { | ||
| position: relative; | ||
| direction: ltr; | ||
| text-align: left; | ||
| height: min-content; | ||
| display: flex; | ||
| overflow-x: hidden; | ||
| flex-flow: column wrap; | ||
| flex-direction: column; | ||
| flex-basis: 70vw; | ||
| flex-grow: 4; | ||
| } |
There was a problem hiding this comment.
suggestion (code_clarification): Consider using a more descriptive class name.
The class name '.code-review-container' could be more specific to its purpose or content, enhancing readability and maintainability.
| @@ -809,7 +878,7 @@ | |||
| */ | |||
| pre[class*="language-"] { | |||
| /* Override prism.js */ | |||
| padding: 0 !important; | |||
| padding: 0.5rem !important; | |||
There was a problem hiding this comment.
suggestion (code_refinement): Use of !important in CSS should be minimized.
Consider alternative methods to increase specificity without using !important, as it can make future CSS maintenance challenging.
| @@ -64,8 +64,7 @@ def _verify_comments(): | |||
| first = auto_comments[0] | |||
| assert first.exercise_test_name.test_name == 'test_check_bar_bar' | |||
| assert first.exercise_test_name.pretty_test_name == 'שם כזה מגניב 2' | |||
| expected = ("AssertionError: איזה ברברון" | |||
| "assert 'bar' == 'barbaron' - barbaron + bar") | |||
| expected = "איזה ברברון" | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for multi-line assertion messages.
The change in services.py now handles assertion messages differently, focusing on the first line only. It would be beneficial to ensure that this behavior is explicitly tested, especially for multi-line assertion messages, to verify that only the intended portion of the message is extracted.
| @@ -64,8 +64,7 @@ | |||
| first = auto_comments[0] | |||
| assert first.exercise_test_name.test_name == 'test_check_bar_bar' | |||
| assert first.exercise_test_name.pretty_test_name == 'שם כזה מגניב 2' | |||
| expected = ("AssertionError: איזה ברברון" | |||
| "assert 'bar' == 'barbaron' - barbaron + bar") | |||
| expected = "איזה ברברון" | |||
There was a problem hiding this comment.
suggestion (testing): Ensure the test covers various assertion message formats.
Given the modification in services.py to extract and format assertion messages, it's crucial to test with various assertion message formats to confirm the robustness of the new logic. This includes testing with messages that do not start with 'AssertionError: ' to ensure they are handled correctly.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #384 +/- ##
=======================================
Coverage 80.43% 80.43%
=======================================
Files 65 65
Lines 3067 3067
=======================================
Hits 2467 2467
Misses 600 600 ☔ View full report in Codecov by Sentry. |
No description provided.