Create test_create_profile.py #2250

SongmingFan123 · 2025-03-10T22:39:02Z

No description provided.

codecov · 2025-03-10T22:54:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.16%. Comparing base (39fb610) to head (b5288ef).
Report is 62 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2250      +/-   ##
==========================================
+ Coverage   75.84%   76.16%   +0.31%     
==========================================
  Files          83       84       +1     
  Lines        3192     3226      +34     
  Branches      348      349       +1     
==========================================
+ Hits         2421     2457      +36     
+ Misses        702      701       -1     
+ Partials       69       68       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

anth-volk

Thanks for this @SongmingFan123. I had some requested changes. Please feel free to reach out with any questions, etc.

anth-volk · 2025-03-11T23:52:05Z

tests/unit/services/test_create_profile.py

+
+class TestCreateProfile:
+
+    def fetch_created_record(self, auth0_id):


issue, blocking: This as a method used in testing could be more brittle and error-prone than fetching data back out of the database itself via an ORM

The issue here is twofold:

You're using one method from this service to validate the other. If that first method works well and never changes, then there'll be no problem, but if the method is ever changed or contains bugs, that will have unanticipated side effects for this test.

By using a method, you move away from imagining the test as a black box that takes inputs and validates their intended outputs. create_profile is written to emit something to a database, and thus the most direct way to test this would be to check that it emitted it to the database.

To move forward here, I'd write a new fixture to fetch records out of the database, then save that fixture in fixtures/services/create_profile.py. Feel free to add any questions here.

anth-volk · 2025-03-11T23:53:01Z

tests/unit/services/test_create_profile.py

+        return userService.get_profile(auth0_id)
+
+    def test_create_profile_valid(self):
+        auth0_id = 'test-auth-id'


nit: I'd be a bit more explicit here and name this something like garbage_auth0_id so as to not imply any formatting to match any internal structure auth0 might provide

anth-volk · 2025-03-11T23:53:20Z

tests/unit/services/test_create_profile.py

+import unittest.mock as mock
+from policyengine_api.services.user_service import UserService
+
+userService = UserService()


nit: Python typically uses snake_case, not camelCase.

anth-volk · 2025-03-11T23:57:16Z

tests/unit/services/test_create_profile.py

+            primary_country=primary_country, auth0_id=auth0_id, username=username, user_since=user_since
+        )
+
+        assert result[0] is True


comment: If fetching directly from the db, this line would be removed

question: What does this line do now?

anth-volk · 2025-03-12T00:04:08Z

tests/unit/services/test_create_profile.py

+        assert user_record['username'] == username
+        assert user_record['user_since'] == user_since
+
+    def test_create_profile_invalid(self):


suggestion: Make this title a bit more descriptive, as there are a few different "invalid" sets of arguments

anth-volk · 2025-03-12T00:05:42Z

tests/unit/services/test_create_profile.py

+        assert user_record['user_since'] == user_since
+
+    def test_create_profile_invalid(self):
+        primary_country = 'United States'


issue: This isn't clear anywhere in the repo, but this is actually formatted as "us" for the US, "uk" for the UK, etc.; could you change this and the other one above to be of that format?

anth-volk · 2025-03-12T00:07:20Z

tests/unit/services/test_create_profile.py

+                primary_country=primary_country, username=username, user_since=user_since
+            )
+
+    def test_create_profile_duplicate(self):


issue, blocking: I'm not sure this test correctly tests the feature

The way I'd do this is to involve the database directly - emit the first one, check that it's present, then emit the second one and make sure that only one record remains

More broadly, what is the expected behavior when a duplicate is emitted? Maybe this should be raising an error or something else?

SongmingFan123 · 2025-03-13T01:27:31Z

Thank you very much Anthony for your detailed instructions and issues with the code. I have just created another PR for the updated code.

SongmingFan123 · 2025-03-13T01:32:23Z

For the database, I used the test_db fixture in the conftest.py within tests/unit.

anth-volk · 2025-03-21T06:56:08Z

@SongmingFan Before I review, could you run make format and add a changelog entry, following the instructions in the README?

SongmingFan123 · 2025-03-21T14:55:58Z

@SongmingFan Before I review, could you run make format and add a changelog entry, following the instructions in the README?

I’ll fix that, thanks for the feedback

SongmingFan123 · 2025-03-24T19:43:03Z

Just pushed the changed I made, thanks

anth-volk · 2025-04-01T22:55:01Z

CHANGELOG.md

+## [3.12.11] - 2025-03-22 13:20:22
+
+### Added
+
+- Create test_create_profile  
+
+## [3.12.10] - 2025-03-10 12:55:10
+
+### Changed
+
+- Update PolicyEngine US to 1.208.0


issue, blocking: Code should not be added to CHANGELOG.md; please remove

Apologies if the instructions in our PR chat were unclear, but only changelog_entry.yaml should be modified when submitting a PR. The information from this file will automatically be added to CHANGELOG.md.

anth-volk · 2025-04-01T22:55:18Z

CHANGELOG.md

@@ -5367,6 +5378,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0



+[3.12.10]: https://github.com/PolicyEngine/policyengine-api/compare/3.12.9...3.12.10


issue, blocking: This code should be removed

This is the same as above

anth-volk · 2025-04-01T22:56:04Z

changelog.yaml

+- bump: patch
+  changes:
+    changed:
+    - Update PolicyEngine US to 1.208.0
+  date: 2025-03-10 12:55:10


issue, blocking: This code should be added to changelog_entry.yaml, not changelog.yaml

The entry will automatically be added to changelog_entry.yaml as part of the GitHub Actions scripts and should not be included here.

anth-volk · 2025-04-01T22:56:27Z

policyengine_api/constants.py

@@ -6,7 +6,7 @@
 POST = "POST"
 UPDATE = "UPDATE"
 LIST = "LIST"
-VERSION = "3.12.9"
+VERSION = "3.12.10"


issue, blocking: This code should not be updated

This will be updated automatically as part of the merge process and should not be modified

anth-volk · 2025-04-01T22:57:31Z

setup.py

+        "policyengine_us==1.208.0",
+        "policyengine_core>=3.12.10",


question: Did you modify this?

If not and all of my comments seem unexpected (I'm starting to suspect they are), I'd recommend switching to your local master branch, running git pull origin master, then switching back to your feature branch and running git rebase master to fix this.

It likely has to do with my local branch. Sorry about that.

anth-volk · 2025-04-02T00:19:59Z

tests/unit/services/test_create_profile.py

+            primary_country=primary_country, auth0_id=auth0_id, username=username, user_since=user_since
+        )
+
+        assert result[0] is True


question: What does this line do now?

anth-volk · 2025-04-02T00:20:52Z

tests/unit/services/test_create_profile.py

+        username = "test_username"
+        user_since = 2025
+
+        def fetch_created_record():


nit, non-blocking: I'd probably not write this as a separate function and just fetch the record back out using this code

anth-volk · 2025-04-02T00:21:20Z

tests/unit/services/test_create_profile.py

+        assert user_record is not None
+        assert (
+            user_record["auth0_id"] == auth0_id
+        )  # Ensure column name is correct


question: What is the purpose of this comment?

anth-volk · 2025-04-02T00:22:00Z

tests/unit/services/test_create_profile.py

+        auth0_id = "test_auth0_id"
+        primary_country = "us"
+        username = "test_username"
+        user_since = 2025


issue, blocking: Here, too, this should be a BIGINT value

tests/unit/services/test_create_profile.py

anth-volk · 2025-04-22T20:57:27Z

Superseded by #2348.

Create test_create_profile.py

631abea

anth-volk requested changes Mar 12, 2025

View reviewed changes

Update test_create_profile.py

b5288ef

Updates for create user profile (changelog entry and formatted)

78dbf1c

anth-volk requested changes Apr 2, 2025

View reviewed changes

anth-volk closed this Apr 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create test_create_profile.py #2250

Create test_create_profile.py #2250

SongmingFan123 commented Mar 10, 2025

codecov bot commented Mar 10, 2025 •

edited

Loading

anth-volk left a comment

anth-volk Mar 11, 2025

anth-volk Mar 11, 2025

anth-volk Mar 11, 2025

anth-volk Mar 11, 2025

anth-volk Apr 2, 2025

anth-volk Mar 12, 2025

anth-volk Mar 12, 2025

anth-volk Mar 12, 2025

SongmingFan123 commented Mar 13, 2025

SongmingFan123 commented Mar 13, 2025

anth-volk commented Mar 21, 2025

SongmingFan123 commented Mar 21, 2025

SongmingFan123 commented Mar 24, 2025

anth-volk Apr 1, 2025

anth-volk Apr 1, 2025

anth-volk Apr 1, 2025

anth-volk Apr 1, 2025

anth-volk Apr 1, 2025

SongmingFan123 Apr 7, 2025

anth-volk Apr 2, 2025

anth-volk Apr 2, 2025

anth-volk Apr 2, 2025

anth-volk Apr 2, 2025

anth-volk commented Apr 22, 2025


		class TestCreateProfile:

		def fetch_created_record(self, auth0_id):

		@@ -5367,6 +5378,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0



		[3.12.10]: https://github.com/PolicyEngine/policyengine-api/compare/3.12.9...3.12.10

Create test_create_profile.py #2250

Create test_create_profile.py #2250

Conversation

SongmingFan123 commented Mar 10, 2025

codecov bot commented Mar 10, 2025 • edited Loading

Codecov Report

anth-volk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SongmingFan123 commented Mar 13, 2025

SongmingFan123 commented Mar 13, 2025

anth-volk commented Mar 21, 2025

SongmingFan123 commented Mar 21, 2025

SongmingFan123 commented Mar 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anth-volk commented Apr 22, 2025

codecov bot commented Mar 10, 2025 •

edited

Loading