DESENG-436: MET - Rewrite unit tests #2358

VineetBala-AOT · 2024-01-12T23:29:23Z

Issue #: https://apps.itsm.gov.bc.ca/jira/browse/DESENG-436

Description of changes:

Resolved API unit test failures on local runs by updating the test suite to pre-create a user before execution.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).

codecov-commenter · 2024-01-12T23:31:33Z

Codecov Report

Attention: 500 lines in your changes are missing coverage. Please review.

Comparison is base (f7e3ac7) 68.56% compared to head (046e134) 67.84%.
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2358      +/-   ##
==========================================
- Coverage   68.56%   67.84%   -0.72%     
==========================================
  Files         472      526      +54     
  Lines       15593    16828    +1235     
  Branches     1193     1230      +37     
==========================================
+ Hits        10691    11417     +726     
- Misses       4693     5185     +492     
- Partials      209      226      +17

Flag	Coverage Δ
analyticsapi	`78.72% <48.00%> (?)`
metapi	`75.64% <55.89%> (-1.14%)`	⬇️
metweb	`60.27% <39.57%> (-1.22%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
.../analytics_api/models/available_response_option.py	`100.00% <100.00%> (ø)`
met-api/src/met_api/__init__.py	`93.50% <100.00%> (-3.80%)`	⬇️
...api/src/met_api/constants/timeline_event_status.py	`100.00% <100.00%> (ø)`
met-api/src/met_api/models/__init__.py	`100.00% <100.00%> (ø)`
met-api/src/met_api/models/base_model.py	`92.59% <100.00%> (+1.21%)`	⬆️
met-api/src/met_api/models/engagement_metadata.py	`60.00% <ø> (-1.54%)`	⬇️
met-api/src/met_api/models/engagement_slug.py	`100.00% <100.00%> (ø)`
met-api/src/met_api/models/feedback.py	`90.90% <100.00%> (+0.16%)`	⬆️
met-api/src/met_api/resources/__init__.py	`100.00% <100.00%> (ø)`
met-api/src/met_api/resources/engagement.py	`83.33% <ø> (ø)`
... and 80 more

... and 36 files with indirect coverage changes

Baelx

Nice work! There's not much to quibble with but I am curious about a few things. If you could please answer my questions before proceeding, I'd appreciate it

Baelx · 2024-01-16T17:05:59Z

met-api/sample.env

@@ -124,6 +124,8 @@ DATABASE_TEST_NAME=
 DATABASE_TEST_HOST=
 DATABASE_TEST_PORT=

+KEYCLOAK_TEST_BASE_URL="http://localhost:8081"


So does this mean one needs to run an instance of keycloak locally just to be able to test?

No need to run a separate instance of Keycloak. The test utilizes a Docker Compose file, and the Keycloak instance is automatically set up when executing the pytest command locally.

Oh okay so if I'm understanding: a limited, temporary instance of keycloak is spun up only for testing?

Baelx · 2024-01-16T17:09:08Z

met-api/src/met_api/services/widget_timeline_service.py

+                                               Role.EDIT_ENGAGEMENT.value), engagement_id=eng_id)
+
+    @staticmethod
+    def _update_widget_timeline(widget_timeline: WidgetTimelineModel, timeline_data: dict):


Why was the update method for timelines moved to the service from the model?

I've noticed a pattern in this app where models sometimes contain all CRUD methods and sometimes contain only some of them, with the service taking over. Do you happen to know the reason for this? In my mind, we'd strive to have all method handlers in the same location (perhaps the model). If there's a good reason for this that I'm missing, I'd be curious to hear it!

We have intentionally designed our models to be minimalist, focusing on retrieving requested data. Not all models support full CRUD operations; for instance, tables like submissions are restricted from updates, and deletion is limited, typically reserved for entities like widgets. All business logic is centralized in the service layer.
Moreover the logic in timeline was causing a cyclic dependency because we had the timeline event service being imported back to the model so I had to move that logic to the service.

Baelx · 2024-01-16T17:17:51Z

met-api/src/met_api/services/widget_timeline_service.py

-                    time = event.get('time'),
-                    position = event.get('position'),
-                    status = event.get('status'),
+                    widget_id=widget_id,


Did the linter ask for the removal of these spaces, is there a rule associated with this relevant to classes? I personally prefer when they're spaced out - to me that's more readable.

Yes, the linter suggested removing these spaces. It follows a Python style guide that recommends no spaces around the equal sign for keyword arguments.

met-api/tests/conftest.py

met-api/tests/unit/api/test_engagement.py

Baelx · 2024-01-16T17:34:31Z

met-api/tests/unit/api/test_survey.py

@@ -39,9 +39,11 @@


 @pytest.mark.parametrize('survey_info', [TestSurveyInfo.survey1])
-def test_create_survey(client, jwt, session, survey_info):  # pylint:disable=unused-argument
+def test_create_survey(client, jwt, session, survey_info,
+                       setup_admin_user_and_claims):  # pylint:disable=unused-argument


Do you know if we need to keep the unused argument around instead of adding a rule to ignore it?

We've included the session argument primarily to satisfy the requirement of the testing framework. Although our code doesn't directly utilize the session within its logic, it's included to facilitate potential database operations that might be required during the test execution. Pylint is complaining about this argument and so we had to add this rule.

Baelx · 2024-01-16T17:45:06Z

met-web/tests/unit/components/engagement/form/edit/EngagementForm.Edit.One.test.tsx

    });

-    test('Can move to links tab', async () => {
+    test('Can move to additional details tab', async () => {


Baelx · 2024-01-16T17:49:33Z

met-web/tests/unit/components/engagement/form/edit/EngagementForm.Edit.Two.test.tsx

@@ -145,7 +145,7 @@ describe('Engagement form page tests', () => {

        await waitFor(() => {
            expect(screen.getByDisplayValue('Test Engagement')).toBeInTheDocument();
-            expect(container.querySelector('span.MuiSkeleton-root')).toBeNull();
+            expect(container.querySelector('span.MuiSkeleton-root'));


Just curious: what does calling expect without a matcher function checks for?

Its just checking if the element exists.

Co-authored-by: Baelx <[email protected]>

sonarcloud · 2024-01-16T18:35:10Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

35 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

* DESENG-436: MET - Rewrite unit tests (#1) Co-authored-by: Baelx <[email protected]> * Update met-api/tests/unit/api/test_engagement.py Co-authored-by: Baelx <[email protected]> --------- Co-authored-by: Baelx <[email protected]>

DESENG-436: MET - Rewrite unit tests (#1)

b4df083

VineetBala-AOT added 2 commits January 12, 2024 23:04

Updating tests for keycloak

c13b9d2

Updating factory scenarios

e5efd89

VineetBala-AOT marked this pull request as draft January 15, 2024 17:13

VineetBala-AOT added 4 commits January 15, 2024 13:22

Fixing linting issue on timeline widget

94589fb

Fixing front end unit tests and api linting

31079e5

Updating changelog

da4aeb8

Fixing linting for web

8cee8a1

VineetBala-AOT force-pushed the Unit-test-fixes branch from bd87b91 to 8cee8a1 Compare January 16, 2024 01:02

Merge branch 'main' into Unit-test-fixes

4f584be

VineetBala-AOT marked this pull request as ready for review January 16, 2024 02:01

Baelx approved these changes Jan 16, 2024

View reviewed changes

VineetBala-AOT and others added 2 commits January 16, 2024 10:34

Update met-api/tests/conftest.py

0dfd587

Co-authored-by: Baelx <[email protected]>

Update met-api/tests/unit/api/test_engagement.py

046e134

Co-authored-by: Baelx <[email protected]>

VineetBala-AOT merged commit 027366f into bcgov:main Jan 16, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DESENG-436: MET - Rewrite unit tests #2358

DESENG-436: MET - Rewrite unit tests #2358

VineetBala-AOT commented Jan 12, 2024

codecov-commenter commented Jan 12, 2024 •

edited

Loading

Baelx left a comment

Baelx Jan 16, 2024

VineetBala-AOT Jan 16, 2024

Baelx Jan 16, 2024

Baelx Jan 16, 2024

VineetBala-AOT Jan 16, 2024

Baelx Jan 16, 2024

VineetBala-AOT Jan 16, 2024

Baelx Jan 16, 2024

VineetBala-AOT Jan 16, 2024

Baelx Jan 16, 2024

Baelx Jan 16, 2024

VineetBala-AOT Jan 16, 2024

sonarcloud bot commented Jan 16, 2024

DESENG-436: MET - Rewrite unit tests #2358

DESENG-436: MET - Rewrite unit tests #2358

Conversation

VineetBala-AOT commented Jan 12, 2024

codecov-commenter commented Jan 12, 2024 • edited Loading

Codecov Report

Baelx 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

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

sonarcloud bot commented Jan 16, 2024

Quality Gate passed

codecov-commenter commented Jan 12, 2024 •

edited

Loading