From 024be4bb85cd141c5bcdd2911fcd508437ad576b Mon Sep 17 00:00:00 2001 From: Sandeep Thalapanane Date: Mon, 5 May 2025 23:56:19 -0400 Subject: [PATCH 1/5] HelpersTask655: Work in Progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-commit checks: - 'check_master' passed - 'check_author' passed - 'check_file_size' passed - 'check_python_compile' passed - 'check_gitleaks' passed All checks passed ✅ --- .../all.write_unit_tests.how_to_guide.md | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/docs/coding/all.write_unit_tests.how_to_guide.md b/docs/coding/all.write_unit_tests.how_to_guide.md index 1f8948172..f47d5638d 100644 --- a/docs/coding/all.write_unit_tests.how_to_guide.md +++ b/docs/coding/all.write_unit_tests.how_to_guide.md @@ -877,6 +877,73 @@ It is best to apply on any part that is deemed unnecessary for specific test - We want to test the minimal amount of behavior that enforces what we care about +3. When to mock (external services only) First, identify the external boundaries + of your code: + + **Bad**: + + ```python + @umock.patch("my_module._process_labels") + def test_sync_labels(self, mock_process): + # Don't mock internal processing + ``` + + **Good**: + + ```python + @umock.patch("github.Github") + def test_sync_labels(self, mock_github): + mock_repo = umock.Mock() + mock_github.return_value.get_repo.return_value = mock_repo + # Test code that uses GitHub API + ``` + +4. What to mock (service clients/interfaces) Next, decide what portion of that + boundary to stub: + + **Bad**: + + ```python + @umock.patch("my_module._make_github_request") + def test_sync_labels(self, mock_request): + # Don't mock how the request is made + ``` + + **Good**: + + ```python + @umock.patch("github.Github") + def test_sync_labels(self, mock_github): + mock_repo = umock.Mock() + mock_github.return_value.get_repo.return_value = mock_repo + # Test code that uses GitHub API + ``` + +5. How to mock (minimal, realistic mocks) Finally, keep your stub lean and + realistic: + + **Bad**: + + ```python + mock_label = umock.Mock() + mock_label.name = "bug" + mock_label.color = "f29513" + mock_label.description = "Something isn't working" + mock_label.created_at = "2024-01-01" + mock_label.updated_at = "2024-01-02" + # ... many more unnecessary attributes + ``` + + **Good**: + + ```python + mock_label = umock.Mock() + mock_label.name = "bug" + mock_label.color = "f29513" + mock_repo = umock.Mock() + mock_repo.get_labels.return_value = [mock_label] + ``` + ### Some general suggestions about testing #### Test from the outside-in From 02e5a9068ed4f35c97be294b05f9d239bcc0efae Mon Sep 17 00:00:00 2001 From: Sandeep Thalapanane Date: Wed, 7 May 2025 00:37:08 -0400 Subject: [PATCH 2/5] HelpersTask655: Add examples for mocking best practies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-commit checks: - 'check_master' passed - 'check_author' passed - 'check_file_size' passed - 'check_python_compile' passed - 'check_gitleaks' passed All checks passed ✅ --- .../all.write_unit_tests.how_to_guide.md | 147 ++++++++++-------- 1 file changed, 81 insertions(+), 66 deletions(-) diff --git a/docs/coding/all.write_unit_tests.how_to_guide.md b/docs/coding/all.write_unit_tests.how_to_guide.md index f47d5638d..11be82250 100644 --- a/docs/coding/all.write_unit_tests.how_to_guide.md +++ b/docs/coding/all.write_unit_tests.how_to_guide.md @@ -868,6 +868,28 @@ It is best to apply on any part that is deemed unnecessary for specific test - We want to replace the provider with an object that responds to the requests with the actual response of the provider - In this way, we can leave all the code of our class untouched and tested + - We want to mock only the external provider; never mock internal helpers + + **Bad**: + + ```python + @umock.patch("helpers.get_labels") + def test_github_labels(self, mock_helper): + # Do not mock internal helper. + ... + ``` + + **Good**: + + ```python + @umock.patch("github.Github") + def test_github_labels(self, mock_github): + # Mock only the external provider. + mock_repo = umock.Mock() + mock_github.return_value.get_repo.return_value = mock_repo + ... + ``` + 2. We want to test public methods of our class (and a few private methods) - In other words, we want to test the end-to-end behavior and not how things are achieved @@ -877,72 +899,65 @@ It is best to apply on any part that is deemed unnecessary for specific test - We want to test the minimal amount of behavior that enforces what we care about -3. When to mock (external services only) First, identify the external boundaries - of your code: - - **Bad**: - - ```python - @umock.patch("my_module._process_labels") - def test_sync_labels(self, mock_process): - # Don't mock internal processing - ``` - - **Good**: - - ```python - @umock.patch("github.Github") - def test_sync_labels(self, mock_github): - mock_repo = umock.Mock() - mock_github.return_value.get_repo.return_value = mock_repo - # Test code that uses GitHub API - ``` - -4. What to mock (service clients/interfaces) Next, decide what portion of that - boundary to stub: - - **Bad**: - - ```python - @umock.patch("my_module._make_github_request") - def test_sync_labels(self, mock_request): - # Don't mock how the request is made - ``` - - **Good**: - - ```python - @umock.patch("github.Github") - def test_sync_labels(self, mock_github): - mock_repo = umock.Mock() - mock_github.return_value.get_repo.return_value = mock_repo - # Test code that uses GitHub API - ``` - -5. How to mock (minimal, realistic mocks) Finally, keep your stub lean and - realistic: - - **Bad**: - - ```python - mock_label = umock.Mock() - mock_label.name = "bug" - mock_label.color = "f29513" - mock_label.description = "Something isn't working" - mock_label.created_at = "2024-01-01" - mock_label.updated_at = "2024-01-02" - # ... many more unnecessary attributes - ``` - - **Good**: - - ```python - mock_label = umock.Mock() - mock_label.name = "bug" - mock_label.color = "f29513" - mock_repo = umock.Mock() - mock_repo.get_labels.return_value = [mock_label] - ``` + **Bad**: + + ```python + @umock.patch("docker.build_container") + @umock.patch("helpers.get_labels") + @umock.patch("github.Github") + def test_github_labels(self, mock_get_labels, mock_build_container, mock_github): + # Don't mock too much, like internal Docker and helper functions. + mock_get_labels.return_value = ["bug"] + mock_build_container.return_value = "image123" + mock_repo = umock.Mock() + mock_github.return_value.get_repo.return_value = mock_repo + ``` + + **Good**: + + ```python + @umock.patch("github.Github") + def test_GitHub_labels(self, mock_github): + # Mock only the behavior that needs to be tested. + mock_repo = umock.Mock() + mock_github.return_value.get_repo.return_value = mock_repo + ... + ``` + +3. We want our mock object to look just real enough for the code to run. + - Include only the attributes or return values your function actually uses. + + **Bad**: + + ```python + mock_label = umock.Mock() + mock_label.name = "bug" + # Don't add unused fields. + mock_label.color = "f29513" + mock_label.description = "Something is not working" + mock_label.created_at = "2024‑01‑01" + mock_label.updated_at = "2024‑01‑02" + .... + + def test_process_labels(mock_repo): + labels = mock_repo.get_labels() + self.assert_equal(labels[0].name, "bug") + ``` + + **Good**: + + ```python + mock_label = umock.Mock() + # Add only the fields the function uses. + mock_label.name = "bug" + mock_repo = umock.Mock() + mock_repo.get_labels.return_value = [mock_label] + ... + + def test_process_labels(mock_repo): + labels = mock_repo.get_labels() + self.assert_equal(labels[0].name, "bug") + ``` ### Some general suggestions about testing From 7160ae4ddecf149e1b2b3c355cd6b933988c8d6d Mon Sep 17 00:00:00 2001 From: Sandeep Thalapanane Date: Wed, 7 May 2025 00:40:30 -0400 Subject: [PATCH 3/5] HelpersTask655: Remove full stops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-commit checks: - 'check_master' passed - 'check_author' passed - 'check_file_size' passed - 'check_python_compile' passed - 'check_gitleaks' passed All checks passed ✅ --- docs/coding/all.write_unit_tests.how_to_guide.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/coding/all.write_unit_tests.how_to_guide.md b/docs/coding/all.write_unit_tests.how_to_guide.md index 11be82250..dff58fc33 100644 --- a/docs/coding/all.write_unit_tests.how_to_guide.md +++ b/docs/coding/all.write_unit_tests.how_to_guide.md @@ -924,8 +924,8 @@ It is best to apply on any part that is deemed unnecessary for specific test ... ``` -3. We want our mock object to look just real enough for the code to run. - - Include only the attributes or return values your function actually uses. +3. We want our mock object to look just real enough for the code to run + - Include only the attributes or return values your function actually uses **Bad**: From f5e41d2880c0bfaa8713ee1c0cc9f1d5b5fca5e0 Mon Sep 17 00:00:00 2001 From: Sandeep Thalapanane Date: Wed, 7 May 2025 00:51:30 -0400 Subject: [PATCH 4/5] HelpersTask655: Change mocking internal helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-commit checks: - 'check_master' passed - 'check_author' passed - 'check_file_size' passed - 'check_python_compile' passed - 'check_gitleaks' passed All checks passed ✅ --- docs/coding/all.write_unit_tests.how_to_guide.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/coding/all.write_unit_tests.how_to_guide.md b/docs/coding/all.write_unit_tests.how_to_guide.md index dff58fc33..d79fcd6de 100644 --- a/docs/coding/all.write_unit_tests.how_to_guide.md +++ b/docs/coding/all.write_unit_tests.how_to_guide.md @@ -868,7 +868,8 @@ It is best to apply on any part that is deemed unnecessary for specific test - We want to replace the provider with an object that responds to the requests with the actual response of the provider - In this way, we can leave all the code of our class untouched and tested - - We want to mock only the external provider; never mock internal helpers + - We want to mock only the external provider and avoid mocking internal + helpers **Bad**: From ed941acedf791b789277fbf6ff120f5ccb77d5e1 Mon Sep 17 00:00:00 2001 From: GP Saggese Date: Thu, 8 May 2025 16:49:20 -0400 Subject: [PATCH 5/5] Improve --- .../all.write_unit_tests.how_to_guide.md | 311 +++++++++--------- 1 file changed, 159 insertions(+), 152 deletions(-) diff --git a/docs/coding/all.write_unit_tests.how_to_guide.md b/docs/coding/all.write_unit_tests.how_to_guide.md index d79fcd6de..9b994b0b7 100644 --- a/docs/coding/all.write_unit_tests.how_to_guide.md +++ b/docs/coding/all.write_unit_tests.how_to_guide.md @@ -96,11 +96,17 @@ - Use Saboteurs to Test Your Testing - Find Bugs Once +- Read these wisdom pearls carefully and you will have made another step towards + programming mastery + ### Unit testing tips #### Test one thing - A good unit test tests only one thing + - A test class should test only one function / class + - A test method should only test a single case (e.g., "for these inputs the + function responds with this output" - Testing one thing keeps the unit test simple, relatively easy to understand, and helps isolate the root cause when the test fails - How do you test more than one thing? By having more than one unit test! @@ -435,6 +441,82 @@ exp = r""" self.assert_equal(act, exp, fuzzy_match=True) ``` +#### Test from the outside-in + +- We want to start testing from the end-to-end methods towards the constructor + of an object +- Rationale: often, we start testing the constructor very carefully and then we + get tired / run out of time when we finally get to test the actual behavior +- Also, testing the important behavior automatically tests building the objects +- Use the code coverage to see what's left to test once you have tested the + "most external" code + +#### We don't need to test all the assertions + +- E.g., testing carefully that we can't pass a value to a constructor doesn't + really test much besides the fact that `dassert` works (which, surprisingly + works!) +- We don't care about line coverage or checking boxes for the sake of checking + boxes + +#### Use strings to compare output instead of data structures + +- Often, it's easier to do a check like: + + ```python + # Better: + expected = str(...) + expected = pprint.pformat(...) + + # Worse: + expected = ["a", "b", { ... }] + ``` + + rather than building the data structure + +- Some purists might not like this, but + - It's much faster to use a string (which is or should be one-to-one to the + data structure), rather than the data structure itself + - By extension, many of the more complex data structure have a built-in + string representation + - It is often more readable and easier to diff (e.g., `self.assertEqual` vs + `self.assert_equal`) + - In case of mismatch, it's easier to update the string with copy-paste rather + than creating a data structure that matches what was created + +#### Use `self.check_string()` for things that we care about not changing (or are too big to have as strings in the code) + +- Use `self.assert_equal()` for things that should not change (e.g., 1 + 1 = 2) +- When using `check_string` still try to add invariants that force the code to + be correct +- E.g., if we want to check the PnL of a model, we can freeze the output with + `check_string()`, but we want to add a constraint like there are more + timestamps than 0 to avoid the situation where we update the string to + something malformed + +#### Each test method should test a single test case + +- Rationale: we want each test to be clear, simple, fast +- If there is repeated code we should factor it out (e.g., builders for objects) + +#### Each test should be crystal clear on how it is different from the others + +- Often, you can factor out all the common logic into a helper method +- Copy-paste is not allowed in unit tests in the same way it's not allowed in + production code + +#### In general, you want to budget the time to write unit tests + +- E.g., "I'm going to spend 3 hours writing unit tests". This is going to help + you focus on what's important to test and force you to use an iterative + approach rather than incremental (remember the Monalisa) + + + +#### Write a skeleton of unit tests and ask for a review if you are not sure how what to test + +- Aka "testing plan" + #### Interesting testing functions - List of useful testing functions are: @@ -510,7 +592,7 @@ self.assert_equal(act, exp, fuzzy_match=True) `super().setUp()`/`super.tearDown()`, then `setUp()`/`tearDown()` can be discarded completely. -##### Nested set_up_test / tear_down_test +#### Nested set_up_test / tear_down_test - When a test class (e.g., TestChild) inherits from another test class (e.g., TestParent), `setUp()`/`tearDown()` methods in the child class normally @@ -842,90 +924,50 @@ self.assert_equal(act, exp, fuzzy_match=True) - Official Python documentation for the mock package can be seen here [unit test mock](https://docs.python.org/3/library/unittest.mock.html) -### Common usage samples - -It is best to apply on any part that is deemed unnecessary for specific test - -- Complex functions - - Mocked functions can be tested separately -- 3rd party provider calls - - CCXT - - AWS - - S3 - - See [`/helpers/hmoto.py`](/helpers/hmoto.py) - - Secrets - - Etc... -- DB calls - -- Many more possible combinations can be seen in the official documentation. -- Below are the most common ones for basic understanding. - -### Philosophy about mocking - -1. We want to mock the minimal surface of a class - - E.g., assume there is a class that is interfacing with an external provider - and our code places requests and gets values back - - We want to replace the provider with an object that responds to the - requests with the actual response of the provider - - In this way, we can leave all the code of our class untouched and tested - - We want to mock only the external provider and avoid mocking internal - helpers - - **Bad**: - - ```python - @umock.patch("helpers.get_labels") - def test_github_labels(self, mock_helper): +### Our Philosophy about mocking + +#### Mock only external dependencies + +- Typically we want to mock interactions with only external components, e.g., + - 3rd party provider + - CCXT + - Cloud infra (e.g., AWS) + - S3 (e.g., see [`/helpers/hmoto.py`](/helpers/hmoto.py)) + - AWS Secrets + ... + - DataBase + - GitHub + - ... + + - E.g., assume there is a class that is interfacing with an external data + provider and our code places requests and gets values back + - We want to replace the provider with an object that responds to the + requests with the actual response of the provider + +- If we want to interactions with GitHub we should mock the GitHub library and + not our API on top of it (since we want to test it) + + **Good**: + + ```python + @umock.patch("github.Github") + def test_github_labels(self, mock_github): + # Mock only the external provider. + mock_repo = umock.Mock() + mock_github.return_value.get_repo.return_value = mock_repo + ... + ``` + + **Bad**: + + ```python + @umock.patch("helpers.get_labels") + def test_github_labels(self, mock_helper): # Do not mock internal helper. - ... - ``` - - **Good**: - - ```python - @umock.patch("github.Github") - def test_github_labels(self, mock_github): - # Mock only the external provider. - mock_repo = umock.Mock() - mock_github.return_value.get_repo.return_value = mock_repo - ... - ``` + ... + ``` -2. We want to test public methods of our class (and a few private methods) - - In other words, we want to test the end-to-end behavior and not how things - are achieved - - Rationale: if we start testing "how" things are done and not "what" is - done, we can't change how we do things (even if it doesn't affect the - interface and its behavior), without updating tons of methods - - We want to test the minimal amount of behavior that enforces what we care - about - - **Bad**: - - ```python - @umock.patch("docker.build_container") - @umock.patch("helpers.get_labels") - @umock.patch("github.Github") - def test_github_labels(self, mock_get_labels, mock_build_container, mock_github): - # Don't mock too much, like internal Docker and helper functions. - mock_get_labels.return_value = ["bug"] - mock_build_container.return_value = "image123" - mock_repo = umock.Mock() - mock_github.return_value.get_repo.return_value = mock_repo - ``` - - **Good**: - - ```python - @umock.patch("github.Github") - def test_GitHub_labels(self, mock_github): - # Mock only the behavior that needs to be tested. - mock_repo = umock.Mock() - mock_github.return_value.get_repo.return_value = mock_repo - ... - ``` - -3. We want our mock object to look just real enough for the code to run +- We want our mock object to look just real enough for the code to run - Include only the attributes or return values your function actually uses **Bad**: @@ -960,83 +1002,48 @@ It is best to apply on any part that is deemed unnecessary for specific test self.assert_equal(labels[0].name, "bug") ``` -### Some general suggestions about testing - -#### Test from the outside-in - -- We want to start testing from the end-to-end methods towards the constructor - of an object -- Rationale: often, we start testing the constructor very carefully and then we - get tired / run out of time when we finally get to test the actual behavior -- Also, testing the important behavior automatically tests building the objects -- Use the code coverage to see what's left to test once you have tested the - "most external" code - -#### We don't need to test all the assertions - -- E.g., testing carefully that we can't pass a value to a constructor doesn't - really test much besides the fact that `dassert` works (which, surprisingly - works!) -- We don't care about line coverage or checking boxes for the sake of checking - boxes - -#### Use strings to compare output instead of data structures - -- Often, it's easier to do a check like: - - ```python - # Better: - expected = str(...) - expected = pprint.pformat(...) +#### Do not mock internal dependencies - # Worse: - expected = ["a", "b", { ... }] - ``` +- In general we don't want to mock any code that is inside our repo, since + - we want to actual test the interaction of different pieces of our code + - it creates maintanance problems - rather than building the data structure +#### Testing end-to-end -- Some purists might not like this, but - - It's much faster to use a string (which is or should be one-to-one to the - data structure), rather than the data structure itself - - By extension, many of the more complex data structure have a built-in - string representation - - It is often more readable and easier to diff (e.g., `self.assertEqual` vs - `self.assert_equal`) - - In case of mismatch, it's easier to update the string with copy-paste rather - than creating a data structure that matches what was created - -#### Use `self.check_string()` for things that we care about not changing (or are too big to have as strings in the code) - -- Use `self.assert_equal()` for things that should not change (e.g., 1 + 1 = 2) -- When using `check_string` still try to add invariants that force the code to - be correct -- E.g., if we want to check the PnL of a model, we can freeze the output with - `check_string()`, but we want to add a constraint like there are more - timestamps than 0 to avoid the situation where we update the string to - something malformed - -#### Each test method should test a single test case - -- Rationale: we want each test to be clear, simple, fast -- If there is repeated code we should factor it out (e.g., builders for objects) - -#### Each test should be crystal clear on how it is different from the others - -- Often, you can factor out all the common logic into a helper method -- Copy-paste is not allowed in unit tests in the same way it's not allowed in - production code +- Often we want to test public methods of our class (and a few private methods) + - In other words, we want to test the end-to-end behavior and not how things + are achieved + - Rationale: if we start testing "how" things are done and not "what" is + done, we can't change how we do things (even if it doesn't affect the + interface and its behavior), without updating tons of methods -#### In general, you want to budget the time to write unit tests + - We want to test the minimal amount of behavior that enforces what we care + about -- E.g., "I'm going to spend 3 hours writing unit tests". This is going to help - you focus on what's important to test and force you to use an iterative - approach rather than incremental (remember the Monalisa) + **Bad**: - + ```python + @umock.patch("docker.build_container") + @umock.patch("helpers.get_labels") + @umock.patch("github.Github") + def test_github_labels(self, mock_get_labels, mock_build_container, mock_github): + # Don't mock too much, like internal Docker and helper functions. + mock_get_labels.return_value = ["bug"] + mock_build_container.return_value = "image123" + mock_repo = umock.Mock() + mock_github.return_value.get_repo.return_value = mock_repo + ``` -#### Write a skeleton of unit tests and ask for a review if you are not sure how what to test + **Good**: -- Aka "testing plan" + ```python + @umock.patch("github.Github") + def test_GitHub_labels(self, mock_github): + # Mock only the behavior that needs to be tested. + mock_repo = umock.Mock() + mock_github.return_value.get_repo.return_value = mock_repo + ... + ``` ### Object patch with return value