Use cases & automated testing recommendations

[stusherwin]

Here are my recommendations around use cases and automated testing resulting from my investigations as part of the Use cases spike. Each one of these has been at least partially implemented as part of this spike, so please feel free to look at the code as a template. I recommend checking out the code locally to review rather than looking in changed files as there are a large amount of code changes (sorry) - partly due to rebasing this PR off the components spike branch.

1. Structure SAPSec.Core by function, not by file type

Since we are going forward with Clean Architecture, and starting to build out a proper domain, it becomes less useful so structure the Core project by file type (interfaces, classes, exensions, models, etc) and more useful to structure by function (e.g. search, similar schools, pagination, sorting etc) - see Screaming architecture

Code that changes together should live together, so code that is related to the same piece of functionality should be kept in the same place. This makes it easier to understand the codebase as a whole when browsing through, and also makes it easier to find the code that is relevant when updating an existing feature. This has not been so much of an issue up to now but as the solution grows in size this will become more and more of a problem.

Before:

After:

Note that I've created a top-level School folder and grouped all school-related functionality under this folder. It may be that we decide that this is unnecessary and each of the folders underneath that should be a top-level folder (e.g. SchoolDetails, SchoolSearch, SchoolSimilarity etc). It may also be that we rename this folder to Domain as it contains most of the domain logic for the service.

It may also be that we decide to create a separate SAPSec.Domain project for these folders to live in. I have not gone that far in this spike but would support this decision.

2. Extract out Data concerns into Data solution folder

There are already multiple projects concerned with processing and generating data files. I propose we create a top-level Data folder for these to live in:

SAPData has here been renamed to SAPSec.SqlGenerator, and SAPSec.ModelGenerator has been renamed to SAPSec.DtoGenerator. These better reflect the purposes of these projects. I also introduce a new SAPSec.Data project, which now holds the dynamically-generated DTO classes (previously under Model in SAPSec.Core) and the IStore (previously IRepository) interfaces. I also introduce implementation projects for these interfaces - SAPSec.Data.Json and SAPSec.Data.Postgresql, these classes were previously part of SAPSec.Infrastructure:

There are a number of reasons for this:

• The DTO (previously Model) classes are tightly coupled to the database structure - in fact are dynamically generated from the database structure. This is a good thing as it eliminates the need to keep the code files in sync with the database views. These DTO files should live next to the code that generates them
• As we start to build out a proper domain, we will need domain entities that are structured around the needs of the business and the functionality being implemented, and be decoupled from the database structure.
• As we start creating these domain entities we will start to see a need for domain repositories. The job of a domain repository is to supply a fully populated domain entity or aggregate root to the use case for processing. This is why I propose renaming the existing IRepository interfaces to IStore, as these interfaces currently return the DTO classes directly, whereas a domain repository will return a domain entity. My proposal is that the domain repositories should implement IRepository, depend on the appropriate IStores supplying the one or more DTO objects needed to populate its domain entity, and encapsulate the logic to map the DTOs to the domain entity.
• I propose we keep both IStore and IRepository interfaces as they allow us to work at both levels of abstraction. Having this IStore allows us to create in-memory implementations of these stores which will be very useful for setting up data in automated tests, and allows the mapping logic in the repositories to be fully tested as part of the automated tests, giving us greater test coverage - this is covered later in the testing recommendations.

3. Create IUseCase<TRequest, TResponse> and add UseCase prefix to existing use cases

This was already a recommendation which we have agreed to implement so I won't go into further details.

4. Convert SearchService to FindASchoolUseCase and FindASchoolSuggestionsUseCase

"Find a school" is the last remaining piece of functionality in the existing code that needs to be implemented as use cases. I propose creating two separate use cases: FindASchoolUseCase and FindASchoolSuggestionsUseCase.

The reason for splitting these into two different use cases is that each one represents a separate path from the database to the UI. They may use the same underlying search index but their needs are different. Each supplies a different set of data fields to the UI, and the data is structured differently - the results are sorted in a different way, filtered differently and search results are paginated whereas suggestions are truncated to 10 results.

I have made a start on the implementation of FindASchoolUseCase which can be used as a starting point for this work. The use case should:

• Implement filtering by Local Authority by taking the filter values as input in the Request object and returning the selected filter values in the Response object (similar to FindSimilarSchoolsUseCase) - filtering is currently implemented at the controller level
• Implement pagination and return a paginated list of results (similar to FindSimilarSchoolsUseCase) - pagination is currently implemented at the controller level
• Implement functionality where if a URN is searched for, a single search result is returned for the school with that URN. The controller should then check to see if a single search result is returned, and if so, redirect to the school page. Currently this is implemented at the controller level, where the controller checks the search query and calls a different service method to get the school by URN and redirect to the school page. This behaviour should be implemented in the use case.

5. Create GetSchoolInfoUseCase and use instead of GetSchoolDetailsUseCase

There are many places where the GetSchoolDetailsUseCase is called from the school page controllers (or the ISchoolDetailsService is called from multiple use cases) to return a SchoolDetails object. This object is then passed to the view, which does not actually use any of the SchoolDetails properties other than the URN or school name. This is inefficient as the SchoolDetailsService has a lot of mapping behaviour which is then not needed, and passing around a complex object where it is not needed obscures the flow of data and makes it harder to refactor to see what data is actually used in different places.

I have created a separate SchoolInfo object:

public record SchoolInfo(string Urn, string Name);

and an accompanying use case GetSchoolInfoUseCase just to get the school name and URN. I then use this in place of SchoolDetails for many of the school page actions, as all they need to do is display the name of the school.

6. Deprecate SAPSec.UI.Tests and SAPSec.Web.Tests

In line with the recommendations from Aasim, and the drive from Scott and Hari around reducing our dependence on UI tests which are intensive to run and expensive to maintain, I propose we deprecate the existing UI/Web test projects, and do not add any further tests to these projects going forward.

SAPSec.UI.Tests

These existing tests test the functionality of the service at the UI level using Playwright. This makes these tests expensive and brittle. Instead, the bulk of our functional testing going forward should be at the use case level, as this is where the main functionality should be implemented. We will still need some UI-level tests but the nature of these will be different, so I propose creating new test projects for these new types of tests. This will allow us to see clearly which tests are implemented using the new approach and which are effectively legacy tests using the old approach. As we refactor the existing functionality we can incrementally review the tests in the above projects, and decide whether to migrate them to the new approach, or delete them.

SAPSec.Web.Tests

These tests are mainly unit tests for controllers, many of which are duplicated in the integration tests. Controllers should not be unit tested, as they should really be kept as simple as physically possible, and just map from use case responses to view models.

Having unit tests for controllers also couples the implementation to a particular controller action or actions (e.g. SchoolSearchController.Index()) rather than a route (e.g. /find-a-school). This makes it hard to restructure controllers that handle a specific route, for example if a particular sub-route needed to be extracted out from the controller into its own controller (e.g. if say /find-a-school/suggest was to be extracted out from SchoolSearchController.Suggest() to SearchSuggestionsController.Index()) It is also difficult to unit test certain action results such as redirection, errors etc.

When it comes to writing a test for a controller, create an integration test rather than a unit test. This allows the functionality to be tested at the route level, and makes sure the whole of the backend and frontend functionality are covered by the test.

UI tests going forward

The three types of UI tests I recommend we adopt going forward are: integration tests, end-to-end tests and accessibility tests. I propose we have a separate test project for each of these types of tests.

7. Recommended changes to integration tests

The current set of integration tests are mostly fine as-is. The purpose of these tests should be to test the integration of the front-end and back-end elements of the service, and they mostly do this, with some recommendations:

1. Use AngleSharp to parse HTML content instead of regular expressions: Regular expressions are unwieldy and hard to read, and also are not generally recommended for parsing HTML or any language. AngleSharp uses a C# implementation of the HTML DOM which allows us to use common DOM functions such as document.QuerySelector() or document.GetElementById(). This is also the pattern used by Microsoft for their integration tests.
   AngleSharp also has some ability to interact with elements on the page and to submit forms, which removes the need to craft a custom request body when testing a form submission:

   [Fact]
   public async Task SearchResultsPage_ViaSubmittingSearchForm_ShouldShowCorrectResultCount()
   {
       var page = await fixture.RequestPageAsync(Routes.FindASchool(), HttpStatusCode.OK);
   
       var searchInput = page.InputWithNameShouldExist("Query");
       searchInput.Value = "school";
   
       var searchButton = page.ButtonWithNameShouldExist("Search");
       var resultsPage = await page.SubmitContainingFormAsync(searchButton, HttpStatusCode.OK);
       outputHelper.WriteLine(resultsPage.Body!.OuterHtml);
   
       var results = resultsPage.ElementShouldExist(".results-bar .app-school-results-count");
       results.TrimmedTextContent().Should().Be("Showing 1 - 10 of 833 schools");
   }
   

   Note the extension methods - as we build out tests we should be extracting common patterns into extension methods in SAPSec.Test.Common that we can use in other tests:

   public static class DocumentExtensions
   {
       public static IElement ElementShouldExist(this IDocument doc, string selector)
       {
           var element = doc.QuerySelector(selector);
           element.Should().NotBeNull();
   
           return element;
       }
   
       public static IHtmlInputElement InputWithNameShouldExist(this IDocument doc, string name)
       {
           return doc.QuerySelector($@"input[name=""{name}""]")
               .Should().NotBeNull().And.BeAssignableTo<IHtmlInputElement>()
               .Subject;
       }
   
       public static IHtmlButtonElement ButtonWithNameShouldExist(this IDocument doc, string name)
       {
           return doc.QuerySelector($@"button[name=""{name}""]")
               .Should().NotBeNull().And.BeAssignableTo<IHtmlButtonElement>()
               .Subject;
       }
   
       public static async Task<IDocument> SubmitContainingFormAsync(this IDocument doc, IHtmlButtonElement button, params HttpStatusCode[] expectedStatusCodes)
       {
           if (!expectedStatusCodes.Any())
           {
               expectedStatusCodes = [HttpStatusCode.OK];
           }
   
           var form = button.Ancestors<IHtmlFormElement>().FirstOrDefault();
           form.Should().NotBeNull();
   
           var document = await form.SubmitAsync(button);
           document.StatusCode.Should().BeOneOf(expectedStatusCodes);
   
           return document;
       }
   }
   

2. Extract system-wide tests: There are a lot of tests that are repeated across multiple pages - e.g. page should respond within a certain amount of time, page should handle multiple concurrent requests, routes should be case insensitive, etc. Currently these are duplicated in the test classes for each page, and not always consistently. In reality if these tests establish a contract that should be followed across all pages, then there should be a single test parameterised by all the routes that should satisfy that test, and these types of tests should be kept together in a class containing system-wide tests. This will also clear up the existing test files to focus on tests unique to the page in question

3. Remove content tests: A Lot of the existing tests merely test whether a certain piece of text content exists on the page. This is not really an integration test, and content should be free to be changed by a content editor without having to deal with breaking tests.

4. Replace hardcoded path strings with Routes helper methods: Many of the existing tests use hardcoded strings for page routes to test, e.g. "/find-a-school" or "/school/123456/school-details". This makes the tests brittle meaning when the route for a page is changed, many tests will fail. Instead, Routes helper methods should be used (see Routes.cs), e.g. Routes.Home or Routes.School("123456"). If there are multiple route parameters to take into account, then create new helpers to take these parameters and generate a route string. For example the /find-a-school route takes a search query parameter and a page number, so rather than manually hardcoding these as /find-a-school?query=searchstring&page=1 everywhere, create a new helper method or an overload on an existing helper method e.g. Routes.FindASchool("searchstring", page: 1) (use parameter names when necessary for clarity):

   public static string FindASchool(string? query = null, int? page = null)
   {
       var queryString =
           (query is not null ? $"&query={query}" : "") +
           (page is not null ? $"&page={page}" : "");
   
       var qs = queryString.Any() ? "?" + queryString.Substring(1) : "";
   
       return $"/find-a-school{qs}";
   }
   

   This creates the further advantage that if the query string parameter name were to change, e.g. from page to p, we only have to change this in one place.

5. Remove #region directives: Regions may seem to structure tests by grouping them into sections, however they introduce friction. For instance when first opening this set of tests it is impossible to see how many tests there are in each section, how long each test is, what structure each test has:

   

   It also makes it difficult to spot similarities in tests by scanning through the test file, and hides tests away so if there are any errors or inconsistencies these wouldn't come to light when touching tests in a different region. Another disadvantage is it makes it difficult to fix errors and to see where the errors are in the page without expanding all the regions:

   

   Instead, test names should be used to group related tests together (this will also ensure they appear together in the Test Explorer). It should not be a problem that test files are big, but if the file truly becomes too big then consider breaking out the test file into separate sub-files, e.g. FindASchoolIntegrationTests.cs and FindASchoolIntegrationTests.Pagination.cs

6. Rename test project to SAPSec.Test.Integration: This is a minor thing but this will allow us to use the pattern SAPSec.SomeProject.Tests to identify unit tests specific to SAPSec.SomeProject, and then SAPSec.Tests.* to identify tests that are not related to a specific project.

8. Introduce SAPSec.Test.EndToEnd

End-to-end tests should take a specific user journey, e.g. searching for school, and interact with the UI using Playwright to follow the journey from start to end. This will usually span multiple pages of the service, but the test should be focused on the particular journey in question. It should focus on happy paths but also likely error paths. For instance, searching for a school using a search term, navigating through search pages, interacting with the map, finding no results for a search term etc.

However, as these tests are brittle and slow-running, they should not be used to test combinations of factors or exhaustive testing of error states - this should be done in the use case tests.

In addition, when using Playwright to interact with the UI, use automation test best practices such as using aria roles or text content to identify elements on the page.

See the following for some example end-to-end tests for the Find a School journey (note there are still more tests to write here!):

[Collection("EndToEndTestsCollection")]
public class FindASchoolEndToEndTests(EndToEndTestsFixture fixture) : EndToEndTests(fixture)
{
    public override async Task InitializeAsync()
    {
        await base.InitializeAsync();
        await NavigateTo(Routes.FindASchool());
    }

    [Fact]
    public async Task SearchForASchoolWithNoResults()
    {
        await Page.GetByLabel("Get school improvement insights", new() { Exact = true }).FillAsync("XXX");
        await Page.GetByRole(AriaRole.Button, new() { Name = "Search" }).ClickAsync();
        await CurrentPageShouldNowBe(Routes.FindASchool(query: "XXX"));
        var errorMessage = Page.GetByRole(AriaRole.Alert);
        await Expect(errorMessage).ToBeVisibleAsync();
        await Expect(errorMessage).ToHaveTextAsync("There is a problem We could not find any schools matching your search criteria");
    }

    [Fact]
    public async Task SearchForASchoolWithMultiplePagesOfResultsAndNavigateBetweenPages()
    {
        await Page.GetByLabel("Get school improvement insights", new() { Exact = true }).FillAsync("School");
        await Page.GetByRole(AriaRole.Button, new() { Name = "Search" }).ClickAsync();
        await CurrentPageShouldNowBe(Routes.FindASchool(query: "School"));
        await Expect(Page.GetByText("Showing 1 - 10 of 833 schools")).ToBeVisibleAsync();

        await Page.GetByText("Next page").ClickAsync();
        await CurrentPageShouldNowBe(Routes.FindASchool(query: "School", page: 2));
        await Page.GetByText("Showing 11 - 20 of 833 schools").IsVisibleAsync();

        await Page.GetByLabel("Page 84").ClickAsync();
        await CurrentPageShouldNowBe(Routes.FindASchool(query: "School", page: 84));
        await Page.GetByText("Showing 831 - 833 of 833 schools").IsVisibleAsync();

        await Page.GetByText("Previous page").ClickAsync();
        await CurrentPageShouldNowBe(Routes.FindASchool(query: "School", page: 83));
        await Page.GetByText("Showing 821 - 830 of 833 schools").IsVisibleAsync();

        await Page.GetByLabel("Page 1").ClickAsync();
        await CurrentPageShouldNowBe(Routes.FindASchool(query: "School", page: 1));
        await Page.GetByText("Showing 1 - 10 of 833 schools").IsVisibleAsync();

        await Page.GetByText("Abbeyfield School").ClickAsync();
        await CurrentPageShouldNowBe(Routes.School("138858"));
    }

    [Fact]
    public async Task SearchForASchoolViaUrn()
    {
        await Page.GetByLabel("Get school improvement insights", new() { Exact = true }).FillAsync("138858");
        await Page.GetByRole(AriaRole.Button, new() { Name = "Search" }).ClickAsync();
        await CurrentPageShouldNowBe(Routes.School("138858"));
    }

    [Fact]
    public async Task SearchForASchoolWithASingleResult()
    {
        await Page.GetByLabel("Get school improvement insights", new() { Exact = true }).FillAsync("Abbeyfield School");
        await Page.GetByRole(AriaRole.Button, new() { Name = "Search" }).ClickAsync();
        await CurrentPageShouldNowBe(Routes.School("138858"));
    }

    [Fact]
    public async Task SearchForASchoolUsingSearchSuggestions()
    {
        await Page.GetByLabel("Get school improvement insights", new() { Exact = true }).FillAsync("Abbeyfield");
        await Page.GetByText("Abbeyfield School").ClickAsync(new() { Force = true });
        await Page.GetByRole(AriaRole.Button, new() { Name = "Search" }).ClickAsync();
        await CurrentPageShouldNowBe(Routes.School("138858"));
    }
}

As with the integration tests extract common patterns as extension methods in SAPSec.Test.Common.

9. Introduce SAPSec.Test.Accessibility

Accessibility tests are still very important, however they serve a different purpose from both integration and end-to-end tests, so I propose we create a new project for these tests. In SAPSec.Test.Accessibility I have copied over the accessibility tests from the deprecated SAPSec.UI.Tests project, but with some important differences:

1. Service-wide accessibility tests - these are tests that should be executed against every page in the service. Many of these currently exist in SAPSec.UI.Tests.AccessibilityTests.SchoolDetailsAccessibilityTests which contains a lot of quite generic tests but hardcoded to the school details page. These have been made generic with a list of test cases for each page that should be tested, for example:

    private static readonly PageTestCase[] AllPagePaths = [
        new(Routes.Home, false),
        new(Routes.Accessibility, false),
        new(Routes.FindASchool(), false),
        new(Routes.School("138858"), false),
        new(Routes.SchoolDetails("138858"), false),
        // TODO: Fill out with all pages from service
    ];
   
    [Theory]
    [MemberData(nameof(AllPages))]
    public async Task AllPages_HaveASingleMainLandmark(string path)
    {
        await NavigateTo(path);
   
        var main = Page.Locator("main");
        var count = await main.CountAsync();
   
        count.Should().Be(1, "Page should have exactly one main landmark");
    }
   
    [Theory]
    [MemberData(nameof(AllPages))]
    public async Task AllPages_HasSkipLink(string path)
    {
        await NavigateTo(path);
   
        var skipLink = Page.Locator(".govuk-skip-link");
        var count = await skipLink.CountAsync();
   
        count.Should().Be(1, "Page should have a skip link");
    }
   
    [Theory]
    [MemberData(nameof(AllPages))]
    public async Task AllPages_SkipLink_TargetsMainContent(string path)
    {
        await NavigateTo(path);
   
        var skipLink = Page.Locator(".govuk-skip-link");
        var href = await skipLink.GetAttributeAsync("href");
   
        href.Should().Be("#main-content", "Skip link should target main content");
    }
   
    [Theory]
    [MemberData(nameof(AllPages))]
    public async Task AllPages_PassAxeAccessibilityChecks(string path)
    {
        await NavigateTo(path);
   
        var results = await Page.RunAxe();
   
        var criticalViolations = results.Violations
            .Where(v => v.Impact == "critical" || v.Impact == "serious")
            .ToList();
   
        criticalViolations.Should().BeEmpty(
            $"Found violations: {string.Join(", ", criticalViolations.Select(v => v.Description))}");
    }
   
    public static TheoryData<string> AllPages()
    {
        var data = new TheoryData<string>();
        foreach (var (path, hasH3Headings) in AllPagePaths)
        {
            data.Add(path);
        }
   
        return data;
    }
   

   Note the page paths are not hard-coded but use the Routes.FindASchool() etc helper methods. We should get in the habit of not hardcoding paths anywhere in the code, whether in tests or otherwise. This is because paths are subject to change, and page structure can change over time. Take as an example the current secondary school page structure /school/{URN}/sub-page - we are going to have to change this eventually to match the new primary school page structure e.g. /school/secondary/{URN}/sub-page, and will have to manually change all these hardcoded paths across all the views and tests. Whereas if all references to paths used these common helper methods than we only need to make the change in a single place.

2. Accessibility tests specific to a specific page, e.g. FindASchoolAccessibilityTests - these are concerned with testing the accessibility of specific functionality which would not be covered by the service-wide tests.