Refactored tests for set policy service (Fixes: #2392) #2339
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2339 +/- ##
==========================================
- Coverage 77.16% 76.51% -0.66%
==========================================
Files 88 91 +3
Lines 3377 3628 +251
Branches 366 407 +41
==========================================
+ Hits 2606 2776 +170
- Misses 703 782 +79
- Partials 68 70 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
To proceed, would you mind creating an issue to add tests to |
anth-volk
left a comment
anth-volk
left a comment
There was a problem hiding this comment.
Thanks for this work @ZeelSatasiya! I've requested a few changes, but hoping to move forward after those.
.DS_Store
Outdated
There was a problem hiding this comment.
issue, blocking: Please delete this artifact
Do you know where this comes from? Could be useful to add to .gitignore, but I'm unfamiliar with it myself
policyengine_api/.DS_Store
Outdated
There was a problem hiding this comment.
issue, blocking: Same as above
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_database(): |
There was a problem hiding this comment.
issue: You shouldn't need this
We already provide test_db as a fixture to all tests within the tests/unit folder, so you shouldn't need this separate mocking feature
| test_policy = {"param": "value"} | ||
| test_label = "new_policy" | ||
| test_country_id = "US" | ||
| test_country_id = "us" |
There was a problem hiding this comment.
question: Why is this change necessary?
If it's to prevent some sort of test failure, then I wonder if there's a more fundamental bug of some sort?
|
|
||
| current_api_version = COUNTRY_PACKAGE_VERSIONS.get(test_country_id) | ||
|
|
||
| # Setup mocks |
There was a problem hiding this comment.
comment: Were you to use the test_db fixture exposed here, you should be able to just fetch the created record back out here, rather than having to set up all these mocks
| json.loads(existing_policy["policy_json"]), | ||
| ) | ||
|
|
||
| # THEN the result should indicate the policy already exists |
There was a problem hiding this comment.
comment: Using the test_db fixture here, you should be able to:
- Create a fixture that just adds your expected record to the database before you start testing
- Attempt to add the record
- (Depending on function behavior) Either expect an error throw or check the output record to make sure that it employed some sort of behavior specific to this situation
ZeelSatasiya
changed the title
anth-volk
left a comment
anth-volk
left a comment
There was a problem hiding this comment.
Pursuant to conversation with @ZeelSatasiya, I think my comments around using the test_db fixture do not make sense for this test file. LGTM!
Fixes: #2392
Refactored tests for set policy service.
Modified fixtures file
Modified set_policy() function to check get(country_id) returning null