-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
course_optimizer_provider tests #36033
Conversation
acdf5d5
to
f317880
Compare
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.
Could you clarify what these tests are testing? Ideally test names should describe expected behavior and act as a kind of documentation for the code. I actually don't understand the code itself in-depth yet and having tests that describe expected behavior in their name will make that clear.
If you want you can use AI and give it the code under test and the linked naming convention file (https://github.com/openedx/frontend-lib-content-components/blob/main/docs/decisions/0006-test-names.rst) and then ask it to generate pytest names (only) for the most important cases and edge cases. That will also show you if you missed a test. (Of course AI makes mistakes but I find it still very helpful with this).
If you look at some of the test names Bernie wrote, you can see how good they are. test_number_of_scanned_blocks_equals_blocks_in_course
tells you exactly what is expected.
MOCK_BLOCK.get_parent.return_value = MOCK_UNIT | ||
|
||
|
||
def test_update_node_tree_and_dictionary(self): |
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.
Can you have a look at https://github.com/openedx/frontend-lib-content-components/blob/main/docs/decisions/0006-test-names.rst and follow the naming conventions there?
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.
Also the convention we use for pytest is that the function docstring describes quite clearly what the unit under test is supposed to do and what we are testing.
self.assertEqual(expected_dictionary, result_dictionary) | ||
|
||
|
||
def test_get_node_path(self): |
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.
See naming comment above
Unit tests for return the Data Transfer Object for course optimizer from backend to frontend.