From 5db2581669e3f45ff73e705cac4576840cbc46df Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 7 Jun 2022 11:14:23 +0100 Subject: [PATCH] Simplify tests for TemplateList class In response to: [^1][^2][^3]. Originally there were three tests: - One was testing the "get_user_template_folders" method directly. - The other two were testing "get_template_folders", which calls the "_user_" method if a user is passed - this is what the tests were primarily checking, by passing or not passing a User object. The two tests of "get_template_folders" also implicitly checked that it filtered folders based on a "parent_folder_id". I wanted to emulate the tests but it makes more sense to simplify them, as the methods are now only used by TemplateList internally. To simplify, we just keep just two tests to check that the overall set of folders is filtered based on the presence of a User. We do lose the implicit check of filtering by "parent_folder_id", but: - We are still checking this implicitly in the sense that the loop to generate the overall set of folders terminates. If the method didn't do any filtering, the loop would be infinite. - We need to acknowledge that these tests were incomplete to begin with. There is also lots of coverage in higher level tests [^4], although it's harder to reason exactly what is covered. [^1]: https://github.com/alphagov/notifications-admin/pull/4258#discussion_r890144076 [^2]: https://github.com/alphagov/notifications-admin/pull/4258#discussion_r890151220 [^3]: https://github.com/alphagov/notifications-admin/pull/4258#discussion_r890147506 [^4]: https://github.com/alphagov/notifications-admin/blob/1787f9f42f0f71854c9fb621c2f0a8f4f7c76286/tests/app/main/views/test_template_folders.py --- app/models/template_list.py | 1 - tests/app/models/test_template_list.py | 45 +++++++------------------- 2 files changed, 11 insertions(+), 35 deletions(-) diff --git a/app/models/template_list.py b/app/models/template_list.py index cfdff1f11..6fc5ab8fa 100644 --- a/app/models/template_list.py +++ b/app/models/template_list.py @@ -264,7 +264,6 @@ class TemplateListFolder(TemplateListItem): super().__init__(folder, ancestors) self.folder = folder self.service_id = service_id - self.folders = folders self.number_of_templates = len(templates) self.number_of_folders = len(folders) diff --git a/tests/app/models/test_template_list.py b/tests/app/models/test_template_list.py index b8a3d5f4f..7aad9959e 100644 --- a/tests/app/models/test_template_list.py +++ b/tests/app/models/test_template_list.py @@ -96,34 +96,7 @@ def test_template_list_yields_folders_visible_to_user( ) -def test_template_list_folder_yields_user_folders_when_user_id_passed_in( - notify_admin, - mock_get_template_folders, - mock_get_service_templates, - service_one, - active_user_with_permissions, - mocker -): - mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) - service = Service(service_one) - user = User(active_user_with_permissions) - - second_parent = next( - result for result in TemplateList(service=service, user=user) - if result.id == VIS_PARENT_FOLDER_ID - ) - - result_folder_names = tuple( - result['name'] for result in second_parent.folders - ) - - assert result_folder_names == ( - "2's Visible child", - ["2's Invisible child", "2's Visible grandchild"], - ) - - -def test_template_list_folder_yields_all_folders_when_user_id_not_passed_in( +def test_template_list_yields_all_folders_without_user( mock_get_template_folders, mock_get_service_templates, service_one, @@ -133,16 +106,20 @@ def test_template_list_folder_yields_all_folders_when_user_id_not_passed_in( mock_get_template_folders.return_value = _get_all_folders(active_user_with_permissions) service = Service(service_one) - second_parent = next( - result for result in TemplateList(service=service) - if result.id == VIS_PARENT_FOLDER_ID - ) - result_folder_names = tuple( - result['name'] for result in second_parent.folders + result.name for result in + TemplateList(service=service) + if result.is_folder ) assert result_folder_names == ( + "Invisible folder", + "Parent 1 - invisible", + "1's Invisible child", + "1's Visible grandchild", + "1's Visible child", + "Parent 2 - visible", "2's Invisible child", + "2's Visible grandchild", "2's Visible child", )