Skip to content

Ref/refactor sorting tests #54

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

Draft
wants to merge 13 commits into
base: dev
Choose a base branch
from

Conversation

aryan-curiel
Copy link
Contributor

In the following PR, the proposed changes are

Changes to the Sorting classes

  • The items kwarg was moved from the sorting class constructor to the sort method. This way, the constructor is used as a configurator (construction) and the same instance of the sorting class can be used for ordering multiple lists
  • Genericity was added to the sorting classes
  • Type hints related to genericity were added to the sorting classes

Test implementation

  • Created a base class with all common test for sorting
  • Created one class per sorting class, instantiating the sorting class instance

Changes related to the preivous tests

  • Tests changing the item types shouldn't be necessary, since these test are for the sorting classes, not for the comp_func. It would be better that the tests are agnostic to this function. SO testing with simple numbers could be enough.
  • Same as above goes for the pivot function
  • It could be better to create tests for specific sorting scenarios, like list with duplicated elems, or empty lists, or single item lists, or already sorted list, reveresed list....
  • It could be better to have unit test, as atomic as possible, and with self descriptive title. (TIP: if you use copilot, this will hel a looooot)
  • Any other sorting test class could implement the base case test, and define the setUpSortingInstance, that should be common to all tests, since the set up params in the constructor shouldn't affect the test escence.
  • Any sorting algorithm specific test, could be added to the specific test case class

@aryan-curiel aryan-curiel marked this pull request as draft February 6, 2023 13:25
Copy link
Owner

@albexl albexl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks amazing @Acuriel. I think it captures the essence of what unit tests should be a lot more than before. Thanks a lot.

One comment though (more like a question):

  • Even though I understand that the sorting instances should be tested agnostically to the pivot function in quicksort or the item types, shouldn't these two things be tested somehow as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants