Initial refactor of SystemRepo & Service unit tests #302 #299

joelvdavies · 2024-06-19T10:57:53Z

Description

Initial refactor example for system repo unit tests. Code is not actually much shorter for these tests, but I think it is more readable and hopefully easier to maintain as the core test functionality is in one place (there are also some additional asserts that were missing before).

SystemService are actually slightly longer now & the e2e tests are a bit shorter.

Suggestions welcome.

Testing instructions

Review code
Check Actions build

Agile board tracking

Closes #302

MatteoGuarnaccia5 · 2024-06-20T08:26:12Z

test/unit/repositories/test_system.py

+
+        self._created_system = self.system_repository.create(self._system_in, session=self.mock_session)
+
+    def call_create_expecting_error(self, error_type: type):


could we maybe abstract testing for errors even further? The same function is there for get, edit, delete. Could save some lines, especially if it is applied across all tests for every entity

We could do, but I am not sure if its in the spirit of this method https://medium.com/@bas.dijkstra54/making-test-frameworks-readable-the-domain-specific-language-c154c9a9abcb. Abstracting further would effectively mean a single function for each test with a bunch of parameters when really they are supposed to be readable through the function names in logical steps. Where right now each are in the format mock, call, assert.

…stem-repo-unit-tests-#90

joelvdavies · 2024-06-20T09:52:18Z

test/unit/repositories/test_system.py

+class SystemRepoDSL:
+    """Base class for SystemRepo unit tests"""
+
+    test_helpers: RepositoryTestHelpers


It might be worth replacing RepositoryTestHelpers with a base class that the same methods, it would save adding fixtures in a setup function?

As discussed, this can be done in a separate PR.

joelvdavies · 2024-06-20T14:50:34Z

test/unit/services/test_system.py

+        self._expected_system_out = MagicMock()
+        self.test_helpers.mock_update(self.mock_system_repository, self._expected_system_out)


This isn't necessary - I could equally use self.mock_system_repository.get.return_value in the assert later instead of storing this which would make it a bit shorter, although similar to a lot of things in this test file, right now the service layer doesn't do much, later we may need to mock like this with realistic data if there was processing after it to test. This may also be more readable here.

joelvdavies · 2024-06-24T14:48:19Z

test/e2e/test_system.py


-    assert response.status_code == 409
-    assert response.json()["detail"] == "A System with the same name already exists within the same parent System"
+class ListDSL(GetBreadcrumbsDSL):


I think its also worth point out I struggled to come up with a consistent naming system throughout these tests, right now its using _system or _systems in the method names to distinguish between these and later ones for items where we may be able to reuse logic but its not clear to me until we get that far so could be a case of renaming these then.

Same for the class and method names - should they be the request type or not? Here I have used the request type for methods (although we could also use create, update etc), but Get and List inside the class names simply because I didn't see the worth in putting System/Systems after each, but again it may be obvious later which is best.

Like you say, it won't be clear until we get that far.

joelvdavies · 2024-06-24T14:53:40Z

test/e2e/test_system.py

+        self.post_system(SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY)
+        system_id = self.post_system(SYSTEM_POST_DATA_ALL_VALUES_NO_PARENT)
+        self.patch_system(system_id, {"name": SYSTEM_POST_DATA_REQUIRED_VALUES_ONLY["name"]})
+        self.check_patch_system_failed_with_message(
+            409, "A System with the same name already exists within the parent System"
+        )


Also worth mentioning things like this I don't think are in the spirit of DSL, as really it would require additional functions with names such as

self.create_system_with_child()
self.move_system_to_previously_created_system()

But I thought that going that far is where the disadvantages of the method start to occur as maintenance becomes more awkward so I thought this could be a decent hybrid instead.

VKTB

This is very good work, thank you! It is so much more readable and I hope that it will be easier to maintain in future.

test/unit/repositories/test_system.py

test/unit/services/test_system.py

test/e2e/test_system.py

VKTB · 2024-06-25T12:02:31Z

test/e2e/test_system.py

+                [system["id"], system["name"]]
+                # When the expected trail length is < the number of systems posted, only use the last
+                for system in self._posted_systems_get_data[
+                    (len(self._posted_systems_get_data) - expected_trail_length) :


Suggested change

(len(self._posted_systems_get_data) - expected_trail_length) :

(len(self._posted_systems_get_data) - expected_trail_length):

This is black's formatter.

🤦‍♂️ never seen this before 😂

Yeah me neither 😄

joelvdavies self-assigned this Jun 19, 2024

joelvdavies force-pushed the refactor-system-repo-unit-tests-#90 branch 2 times, most recently from abe988e to 7b6f22a Compare June 19, 2024 13:39

joelvdavies requested review from VKTB and MatteoGuarnaccia5 June 19, 2024 15:00

Initial refactor of SystemRepo unit tests #90

28b3440

joelvdavies force-pushed the refactor-system-repo-unit-tests-#90 branch from 7b6f22a to 28b3440 Compare June 19, 2024 15:46

MatteoGuarnaccia5 reviewed Jun 20, 2024

View reviewed changes

Merge branch 'remove-request-from-schema-names-#297' into refactor-sy…

7bf025c

…stem-repo-unit-tests-#90

joelvdavies commented Jun 20, 2024

View reviewed changes

Refactor SystemService unit tests #90

c239713

joelvdavies changed the title ~~Initial refactor of SystemRepo unit tests #90~~ Initial refactor of SystemRepo & Service unit tests #90 Jun 20, 2024

Base automatically changed from remove-request-from-schema-names-#297 to develop June 20, 2024 14:19

Improve parameter & mock data names and use common data file #90

86289a6

joelvdavies commented Jun 20, 2024

View reviewed changes

Initial refactor for systems e2e tests #302

5f2f03b

joelvdavies changed the title ~~Initial refactor of SystemRepo & Service unit tests #90~~ Initial refactor of SystemRepo & Service unit tests #302 Jun 24, 2024

joelvdavies added 3 commits June 24, 2024 11:29

Move all mock data for system tests to same place and improve names #302

4bb51da

Fix test interference and remove unnecessary mock data #302

3155dec

Add some comments and improve name consistency #302

9e25e1a

joelvdavies commented Jun 24, 2024

View reviewed changes

joelvdavies marked this pull request as ready for review June 25, 2024 09:06

VKTB reviewed Jun 25, 2024

View reviewed changes

Fixes for various type errors addressing comments #302

03920e1

joelvdavies force-pushed the refactor-system-repo-unit-tests-#90 branch from 53bc563 to 03920e1 Compare June 25, 2024 09:42

Add comment on wrapped utils and a todo as suggested #302

17c1397

joelvdavies requested a review from VKTB June 25, 2024 11:05

VKTB reviewed Jun 25, 2024

View reviewed changes

joelvdavies requested a review from VKTB June 25, 2024 12:48

VKTB approved these changes Jun 25, 2024

View reviewed changes

joelvdavies merged commit efe2c32 into develop Jun 25, 2024
6 checks passed

joelvdavies deleted the refactor-system-repo-unit-tests-#90 branch June 25, 2024 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial refactor of SystemRepo & Service unit tests #302 #299

Initial refactor of SystemRepo & Service unit tests #302 #299

joelvdavies commented Jun 19, 2024 •

edited by VKTB

Loading

MatteoGuarnaccia5 Jun 20, 2024

joelvdavies Jun 20, 2024

joelvdavies Jun 20, 2024

VKTB Jun 25, 2024

joelvdavies Jun 20, 2024 •

edited

Loading

joelvdavies Jun 24, 2024 •

edited

Loading

VKTB Jun 25, 2024

joelvdavies Jun 24, 2024

VKTB left a comment

VKTB Jun 25, 2024

joelvdavies Jun 25, 2024

VKTB Jun 25, 2024

joelvdavies Jun 25, 2024


		self._created_system = self.system_repository.create(self._system_in, session=self.mock_session)

		def call_create_expecting_error(self, error_type: type):

		self._expected_system_out = MagicMock()
		self.test_helpers.mock_update(self.mock_system_repository, self._expected_system_out)

	(len(self._posted_systems_get_data) - expected_trail_length) :
	(len(self._posted_systems_get_data) - expected_trail_length):

Initial refactor of SystemRepo & Service unit tests #302 #299

Initial refactor of SystemRepo & Service unit tests #302 #299

Conversation

joelvdavies commented Jun 19, 2024 • edited by VKTB Loading

Description

Testing instructions

Agile board tracking

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

joelvdavies Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

joelvdavies Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VKTB 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

joelvdavies commented Jun 19, 2024 •

edited by VKTB

Loading

joelvdavies Jun 20, 2024 •

edited

Loading

joelvdavies Jun 24, 2024 •

edited

Loading