Skip to content
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

[ To Main ] - DESENG-508 - Updated Keycloak unit test for new CSS API #2443

Merged
merged 7 commits into from
Apr 5, 2024

Conversation

ratheesh-aot
Copy link
Collaborator

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

Description of changes:
Updated Keycloak Unit Tests for New CSS API Integration.

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).

Copy link

sonarqubecloud bot commented Apr 4, 2024

Quality Gate Passed Quality Gate passed

Issues
8 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.66%. Comparing base (f3da667) to head (1e46366).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2443      +/-   ##
==========================================
+ Coverage   74.77%   75.66%   +0.88%     
==========================================
  Files         577      577              
  Lines       20578    20563      -15     
  Branches     1494     1494              
==========================================
+ Hits        15388    15558     +170     
+ Misses       4956     4771     -185     
  Partials      234      234              
Flag Coverage Δ
metapi 87.68% <ø> (+1.79%) ⬆️

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

Files Coverage Δ
met-api/src/met_api/services/keycloak.py 98.50% <ø> (+68.01%) ⬆️

... and 10 files with indirect coverage changes

@Baelx
Copy link
Collaborator

Baelx commented Apr 4, 2024

@ratheesh-aot So given the recent development that we won't be using the CSS API anymore, do you think any of this work can be preserved for later? 😓

@ratheesh-aot
Copy link
Collaborator Author

@Baelx Some API tests can be useful again. Also to make sure all tests are working until we start on the new Role ticket and to make sure some redundant "add to group " functionalities are removed, we may merge this PR for now. And update it later

Copy link
Contributor

@NatSquared NatSquared left a comment

Choose a reason for hiding this comment

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

These are some really high-quality tests! Thank you :3

KEYCLOAK_SERVICE = KeycloakService()


@pytest.fixture()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this patch intended to run before each function? If so, maybe explicitly specify fixture(scope='function'). If not, specify another scope such as module

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@NatSquared Thanks for notifying this. When I checked the default scope of the fixture will be function always. We need that because we have a test named def test_get_admin_token(mock_post) that checks whether _get_admin_token is called once or not. Also, all other test functions require this patch. That is why I left it to default scope which is function.

@@ -76,31 +75,6 @@ def get_users_roles(self, user_ids: List):

return user_role_mapping

# @staticmethod
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for cleaning up all these comments!

@ratheesh-aot ratheesh-aot merged commit 1a92863 into bcgov:main Apr 5, 2024
11 checks passed
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.

4 participants