Dry specialist publisher tests #3029

GDSNewt · 2025-02-26T18:29:33Z

Adding a separate test suite for each new finder is inefficient, especially when testing the same underlying code.

This PR consolidates multiple test files into a few test suites that iterate across all schema formats. This helps standardise finder tests, improve coverage, and automate more of the process for developers.

Things to note

Not a full refactor – Some document types don’t easily fit into a general test suite. These have been left unchanged and added to an exceptions list.
No new tests added – This PR focuses solely on consolidating existing tests, not filling coverage gaps.
Inconsistent lifecycle testing – While this improves coverage of the creation stage, post-creation actions are still primarily covered by the CMA cases tests.
May increase complacency - there is some reassurance in writing tests manually. There are scenarios where we may erroneously add, or forget to add, validation to our schemas. The automated tests will not be aware of such errors. Having said that, those issues should be picked up when testing the finder in integration.

Trello

Simplifies tests by: - Removing StatutoryInstrument access control tests, and AAIB access control tests. These tests didn't seem to be testing any behaviour different to the existing CMA access tests. - I understand the logic behind testing the 'inverse' of the CMA tests (i.e. that an AAIB editor can access AAIB and not CMA, to verify that the CMA cases aren't "failing open") but given we don't test access controls of ANY of the other document types, it doesn't make a lot of sense to retain anything other than the simple set of CMA access tests. - Moves the general permissions error to the top of the file. It is the simplest case, so should really come first. - Also adds a proper explanation of what's being tested.

Have consolidated a lof of the 'create an x' test logic into one general test file. The test suite includes the following scenarios: - Getting to the new document page. - Creating a document with valid data. - Attempting to create a document with no data. - Attempting to create a document with invalid content. - Attempting to create a document with an invalid date. There are scenarios that have been removed in said consolidation, such as those around the 'open before closed date' validation testing or test around permissions. In the case of the former this has been moved to a seperate test file, all the other scenarios I judged there to be suffcient coverage in existing tests (eg access_control). The test suite dynamically generates tests for each document schema found in the "lib/documents/schemas" directory, except for those listed in the EXCEPTIONS_TO_GENERAL_TESTING constant. The exceptions constant is mostly there to account for the 5 finders that do not yet used the shared form. The other finder listed is there because it isn't live 'ai_assurance_portfolio_technique'. There are some bespoke elements in this test which may raise eyebrows. The Marine equipment and protect food finders have custom validation on some of their fields, I judged it was not too intrusive to account for this in the valid data test - hence the case-when statement. In addition, the base path for the product recall finder differs from the rest of the finders in that is was not just the format type pluralised. It has an s on the end of alerts. I have accounted for this in the decribe block, it seemed to me to be the least intrusive way of handling the issue

Given that we have adopted the apporach of making model validation schema driven we should make sure these validations are actually being applied on the models. It's possible this was overlooked as they were in classes that did not have tests for their validation previously.

Now that these finders are using the same shared form, there's little value in repeating the same tests across them all. I've left a subset of "creating a X specialist document" test files which all exhibit slightly different things - these should be tidied up and consolidated later.

Historically we haven't really tested multiple document types beyond the creation phase. We have been content to use CMA Cases as the test case for editing, viewing, publishing, searching etc. Ideally we would test theses phases for each document type, but for now it feels odd that there are two 'editing tests' for finders that aren't CMA cases, especially as there is nothing special about these finders. I also think there is a case for removing the 'visiting the app' test file, it is outdated and its scenarios are covered by other tests.

Our validations are now largely defined by the schemas and are being applied to the models without the need to be manually defined and configured. This validation in tested by our feature tests. It seems unnecessary to test this validation twice.

ChrisBAshton

This is looking great, Alex - exactly the sort of thing I had in mind! Some comments below.

It would also be good to have an updated idea of what needs doing in the test space when creating a new finder. Are you drafting an update to https://docs.publishing.service.gov.uk/repos/specialist-publisher/creating-editing-and-removing-specialist-document-types-and-finders.html?
I've just raised PR 3011 to add the HMRC Finder and from a quick look at this PR, I think all I'll need to do is add a factory for it - is that right?

ChrisBAshton · 2025-02-28T12:19:56Z

lib/documents/schemas/esi_funds.json

@@ -180,7 +180,7 @@
    }
  ],
  "filter": {
-    "format": "european_structural_investment_fund"
+    "format": "esi_fund"


We can't (easily) change this as this value is used in Search API too and is associated with all the ESI specialist documents.

Yeah was worried that might be the case, format was a convenient value to use from the schema to set the document type in the test. I'm trying to add a conditional, in the test, to do this but having some issues accessing DOM elements atm 😢

spec/features/creating_a_new_document_spec.rb

ChrisBAshton · 2025-02-28T12:35:24Z

spec/models/business_finance_support_scheme_spec.rb

-  let(:payload) { FactoryBot.create(:business_finance_support_scheme) }
-  include_examples "it saves payloads that are valid against the 'specialist_document' schema"
-
-  it "is exportable" do


I think we need to keep these tests, as we're not testing self.exportable? anywhere else.

(We could remove these tests if we write a test that an exportable boolean in the schema can be surfaced by <Finder>.exportable? but it doesn't look like that property exists in the schema yet - it's hardcoded in the model).

Ah yep, you're right. Think I removed by mistake!

ChrisBAshton · 2025-02-28T12:37:15Z

spec/models/document_type_spec.rb

I'd move this into its own commit - it'll make the PR easier to review.

I had drafted the following comment further up in this commit, because it looked as though you were deleting the schema validation tests without adding a new equivalent.

When you move this into its own commit, you can also use that as an opportunity to explain (in the message) why ai_assurance_portfolio_technique needs to be excluded.

I think we'll want to copy this sort of general schema validation check over to `spec/features/creating_a_new_document_spec.rb` (or some other generic test file). We want to ensure that all finder schemas are valid.

ChrisBAshton · 2025-02-28T12:38:16Z

spec/models/protected_food_drink_name_spec.rb

-    subject(:document) { ProtectedFoodDrinkName.new(title: "Черноморски район") }
-
-    it "has a valid base_path" do
-      expect(document.base_path).to eq("/protected-food-drink-names/chiernomorski-raion")


Hmmm we don't have an equivalent for that sort of test, do we? May need to keep.

And if it isn't specific to this finder (from a quick look, doesn't seem to be) then we should also consider moving this into a generic test somewhere.

ChrisBAshton · 2025-02-28T12:39:33Z

spec/models/residential_property_tribunal_decision_spec.rb

-    it "is invalid if the tribunal decision sub category is missing" do
-      tribunal_decision.tribunal_decision_sub_category = nil
-      expect(tribunal_decision).not_to be_valid
-    end
-
-    it "is invalid if the tribunal decision sub category does not match up to the tribunal decision category" do
-      tribunal_decision.tribunal_decision_category = "rents"
-      tribunal_decision.tribunal_decision_sub_category = "tenant-associations---request-for-information"
-      expect(tribunal_decision).not_to be_valid
-    end
-  end


Does this have an equivalent in the general tests? From a quick look, it seems this should be kept.

ChrisBAshton · 2025-02-28T12:40:48Z

spec/models/trademark_decision_spec.rb

-      expect(trademark_decision).to be_valid
-    end
-
-    it "is invalid if the organisation is missing" do


Agree this sort of thing should go - but we should probably have a generic test that a facet marked as 'required' must have a value specified.

lauraghiorghisor-tw

Left some comments, the one requiring change is around the presence validation tests which is currently not running anything.

I second Chris's recommendation around documentation. I'd particularly like to know what the best approach is for adding anything custom. Or what a TDD flow can look like. BUT, maybe that's something we first figure out by using the repo for a while and then update the docs ;)

Since these tests are now running a bit magically perhaps improving the visualisation of the test run could help. Maybe we can change the .rspec development environment settings to run with --format doc flag on. This would then render as below:

.

lauraghiorghisor-tw · 2025-03-03T10:29:56Z

spec/features/creating_a_new_document_spec.rb

+        properties = facet["specialist_publisher_properties"] || {}
+        validations = properties["validations"] || {}
+
+        if validations["presence"]


This test doesn't actually test anything, since this line will never evaluate to true as there is no presence key in the schema. Apologies, I have called it required, just to be fancy 🙈

lauraghiorghisor-tw · 2025-03-03T10:31:28Z

spec/features/creating_a_new_document_spec.rb

+      click_button "Save as draft"
+
+      expect(page.status_code).to eq(200)
+      expect(page).to have_content("Created Example #{document_type.to_s.humanize}")


Since we're looping through all the facets and filling them in, it feels a bit like we've done that for nothing if we don't also include any expectations on that data.

Nonetheless, expecting on ALL of that might slow down things a little bit - perhaps it's enough to only fill out only the required fields at this point and thus expect that we have created the document only - or also expect on the filled in fields.

lauraghiorghisor-tw · 2025-03-03T10:31:43Z

spec/features/creating_a_new_document_spec.rb

+      schema.facets.each do |facet|
+        key = facet["key"]
+        properties = facet["specialist_publisher_properties"] || {}
+        properties["validations"] || {}


Redundant line?

lauraghiorghisor-tw · 2025-03-03T10:32:03Z

app/models/ai_assurance_portfolio_technique.rb

@@ -1,4 +1,5 @@
 class AiAssurancePortfolioTechnique < Document
+  apply_validations


We did not add apply_validations to models that simply did not have any validation. They did not have any presence checks, nor any date fields (the only schema validations we added).

I believe it is still redundant to add this. I understand the "completeness" aspect, but it might also read as, "ah this thing should have validations", but then you look in the schema and there are none 🕵🏻‍♀️ Up to you though.

lauraghiorghisor-tw · 2025-03-03T10:32:51Z

spec/features/creating_a_new_document_spec.rb

+    next if EXCEPTIONS_TO_GENERAL_TESTING.include?(format)
+
+    describe "Creating a #{format.humanize}" do
+      base_path = format == "product_safety_alert_report_recall" ? "/product-safety-alerts-reports-recalls" : "/#{format.pluralize.dasherize}"


The one convention we do uphold across the codebase, though I'd be hard pressed to tell you why it matters that we do so..., is that our models are called, for example ProductSafetyAlertReportRecall which must (as per Rails) live in a file called product_safety_alert_report_recall.rb whose corresponding json file is called product_safety_alert_report_recalls.json - PLURALIZED.

You'd need to do a bit of string manipulation here, but it might be a good bet to use the file variable here rather than the format from the schema 🤔

That should also fix your esi_funds.

Alternatively we can maintain a manual map of formats for the test.

lauraghiorghisor-tw · 2025-03-03T10:58:22Z

spec/features/creating_a_cma_case_spec.rb

-    end
-  end
-
-  scenario "a draft with the same path as an existing draft" do


Not carried over.

spec/features/access_control_spec.rb

lauraghiorghisor-tw · 2025-03-03T11:24:44Z

spec/features/creating_a_residential_property_tribunal_decision_spec.rb

-    expect(page.status_code).to eq(422)
-
-    expect(page).to have_content("Body cannot include invalid Govspeak")
-    expect(page).to have_content("Tribunal decision sub category must belong to the selected tribunal decision category")


Noting the ResidentialPropertyTribunalDecisionSubCategoryValidator test here - but this validator does have unit tests so probably ok.

lauraghiorghisor-tw · 2025-03-03T11:31:50Z

spec/features/creating_a_new_document_spec.rb

+].freeze
+
+RSpec.feature "Creating a document", type: :feature do
+  shared_context "common setup" do |editor, document_type, document_path, new_document_path|


From what I've seen around, it is customary to have the shared context in its own file. Would also mean that it's a tad easier to get to the important part of this file, which uses the shared context some 100 lines down.

lauraghiorghisor-tw · 2025-03-03T11:38:31Z

spec/features/creating_a_new_document_spec.rb

+      expect(page).to have_content("Body cannot include invalid Govspeak")
+    end
+
+    scenario "with invalid date" do


Date validation is applied universally based on the type of the facet in the schema. Perhaps it is not that much value iterating the check per document type.

Maybe it suffices to extract this into its own feature spec file, as you did with open before close validation. And it would make it more visible that that is where our date validation logic is checked.

GDSNewt force-pushed the DRY-specialist-publisher-tests branch 6 times, most recently from 76ee600 to d6fd105 Compare February 28, 2025 11:59

GDSNewt marked this pull request as ready for review February 28, 2025 12:01

GDSNewt and others added 5 commits February 28, 2025 12:30

GDSNewt force-pushed the DRY-specialist-publisher-tests branch from d6fd105 to 0f653f3 Compare February 28, 2025 12:30

ChrisBAshton reviewed Feb 28, 2025

View reviewed changes

lauraghiorghisor-tw requested changes Mar 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dry specialist publisher tests #3029

Dry specialist publisher tests #3029

GDSNewt commented Feb 26, 2025 •

edited

Loading

ChrisBAshton left a comment

ChrisBAshton Feb 28, 2025

GDSNewt Feb 28, 2025 •

edited

Loading

ChrisBAshton Feb 28, 2025

GDSNewt Feb 28, 2025

ChrisBAshton Feb 28, 2025

ChrisBAshton Feb 28, 2025

ChrisBAshton Feb 28, 2025

ChrisBAshton Feb 28, 2025

lauraghiorghisor-tw left a comment

lauraghiorghisor-tw Mar 3, 2025

lauraghiorghisor-tw Mar 3, 2025 •

edited

Loading

lauraghiorghisor-tw Mar 3, 2025

lauraghiorghisor-tw Mar 3, 2025 •

edited

Loading

lauraghiorghisor-tw Mar 3, 2025

lauraghiorghisor-tw Mar 3, 2025

lauraghiorghisor-tw Mar 3, 2025

lauraghiorghisor-tw Mar 3, 2025

lauraghiorghisor-tw Mar 3, 2025

		@@ -1,4 +1,5 @@
		class AiAssurancePortfolioTechnique < Document
		apply_validations

Dry specialist publisher tests #3029

Are you sure you want to change the base?

Dry specialist publisher tests #3029

Conversation

GDSNewt commented Feb 26, 2025 • edited Loading

ChrisBAshton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GDSNewt Feb 28, 2025 • edited Loading

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

lauraghiorghisor-tw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauraghiorghisor-tw Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauraghiorghisor-tw Mar 3, 2025 • edited Loading

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

GDSNewt commented Feb 26, 2025 •

edited

Loading

GDSNewt Feb 28, 2025 •

edited

Loading

lauraghiorghisor-tw Mar 3, 2025 •

edited

Loading

lauraghiorghisor-tw Mar 3, 2025 •

edited

Loading