-
Notifications
You must be signed in to change notification settings - Fork 5
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
EES-5632 Distinguishing release redirects across different publications #5446
EES-5632 Distinguishing release redirects across different publications #5446
Conversation
bc87af9
to
9d368fd
Compare
src/GovUk.Education.ExploreEducationStatistics.Content.Model/MethodologyVersion.cs
Show resolved
Hide resolved
src/explore-education-statistics-frontend/src/middleware/pages/redirectPages.ts
Show resolved
Hide resolved
/// <summary> | ||
/// Start the Azurite container. Once started, the test app must also | ||
/// be configured with <see cref="WithAzurite"/> to use it. | ||
/// </summary> | ||
/// <remarks> | ||
/// We don't start the Azurite container in a class fixture as there currently | ||
/// isn't a good way to clear it after each test. The current approach is to | ||
/// restart the container for each test case (which is quite slow). | ||
/// See: https://github.com/Azure/Azurite/issues/588. | ||
/// For now, we should manually control the Azurite container's lifecycle by | ||
/// calling this on a case-by-case basis. | ||
/// </remarks> | ||
public async Task StartAzurite() | ||
{ | ||
TestApp = testApp; | ||
await _azuriteContainer.StartAsync(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be worth creating a new ticket on this when you get chance, if one doesn't already exist.
I.e. one that explains this downside you mentioned in the description
One downside is that running the tests will be slightly slower due to spinning up a new container for every test - however, being that there are currently very minimal tests actually using this TestContainer, the impact is small.
and also capture the possible solutions you came up with in that ticket, just so we don't forget about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean keep the change in this PR, but create a Jira ticket linked to this ticket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant, when you get chance to create it. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good shout, I'll get one added :)
src/GovUk.Education.ExploreEducationStatistics.Content.Services.Tests/RedirectsServiceTests.cs
Outdated
Show resolved
Hide resolved
...on.ExploreEducationStatistics.Content.Model.Tests/Fixtures/MethodologyGeneratorExtensions.cs
Outdated
Show resolved
Hide resolved
...on.ExploreEducationStatistics.Content.Model.Tests/Fixtures/MethodologyGeneratorExtensions.cs
Outdated
Show resolved
Hide resolved
...ucation.ExploreEducationStatistics.Content.Api.Tests/Controllers/RedirectsControllerTests.cs
Outdated
Show resolved
Hide resolved
...ucation.ExploreEducationStatistics.Content.Api.Tests/Controllers/RedirectsControllerTests.cs
Outdated
Show resolved
Hide resolved
…ed across each test so that the stored data doesn't interfere with other tests
56cf3c6
to
eb5ac48
Compare
The main work for this ticket was completed in PR #5391. However, I failed to realise that we had no way to distinguish between release redirects for different publications. It is possible to have 2 release redirects that have the same
FromSlug
, but they're for different releases - but the frontend had no way of determining which one was for which release (and hence, publication).In this PR, we have made a change to the redirects view-model such that we now have the information to hand to be able to distinguish which release redirects apply to which publication.
The old JSON, as of PR #5391, looked like:
The new JSON, as of this PR, looks like:
Things to note:
releaseRedirectsByPublicationSlug
groups the release redirects by publication, keyed on the LATEST publication slug for the corresponding publication.Other Changes
RedirectsCacheService.cs
) was being carried over between tests, and was interfering with their integrity. This is because we were using an Azurite TestContainer which currently has no built-in support for clearing up the test data stored within the container between tests. See this issue.We previously opted for an approach of starting and stopping the container between test runs. However, this does not actually clear the test data.
One solution is to delete the container's Azurite related files/folders, mentioned here - however, this proved non-trivial to do. We also could not just
Dispose
of the TestContainer between tests, as this leads to anCannot access a disposed object
error between tests within the same test class.Another solution is to place every test inside it's own dedicated test class (one test per class), such that the
TestApplicationFactory
fixture instance, which is normally re-used between every test WITHIN a test class, is only used once. This way, we can safely dispose of it after the test run and know we won't be reusing it. However, this is not the cleanest approach.The solution we finalised on was to move the declaration of the TestContainer up to the
IntegrationTestFixture
. The XUnit framework creates a new instance of this for every test, regardless of whether or not the tests are within the same test class. We then call dispose on the container after each test run and clear the data. Each test uses a different Azurite container instance, and therefore no data leakage occurs. One downside is that running the tests will be slightly slower due to spinning up a new container for every test - however, being that there are currently very minimal tests actually using this TestContainer, the impact is small.