Skip to content

Elig 3501 test suite for internal wf checks - #533

Open
LiliNedeleva wants to merge 13 commits into
mainfrom
ELIG-3501-test-suite-for-internal-wf-checks
Open

Elig 3501 test suite for internal wf checks#533
LiliNedeleva wants to merge 13 commits into
mainfrom
ELIG-3501-test-suite-for-internal-wf-checks

Conversation

@LiliNedeleva

Copy link
Copy Markdown
Contributor
  1. Create new test factories for generating standard and working family test cases
  2. Move and tidy up old code to factories and make the accessible through the gateway
  3. Centralise all test configurations in one place. Ensure dictionaries are safely populated.
  4. Create new test scenarios for internal checks

@YiannosGeorgantas

Copy link
Copy Markdown
Contributor

Could we add unit tests around WorkingFamiliesTestScenarioFactory? I can only see the existing helper tests being adjusted, but the new factory introduces the 700–704 scenarios, term-dependent date generation, DVSD and reconfirmation variations, and configurable code types. I think we particularly need coverage for each scenario across the Spring, Summer and Autumn boundaries, the 704 expired path, “cannot be used yet” later in a term, and a gateway test proving that genuine non-test codes still reach ECS/ECE. Injecting the current date and avoiding random dates would also make these tests deterministic.

Comment thread CheckYourEligibility.API/Gateways/CheckingEngineGateway.cs Outdated
Comment thread CheckYourEligibility.API/Gateways/Factories/WorkingFamiliesTestScenarioFactory.cs Outdated
Comment thread CheckYourEligibility.API/appsettings.json Outdated
Comment thread CheckYourEligibility.API/Helpers/WorkingFamiliesCheckHelper.cs
Comment thread CheckYourEligibility.API/Gateways/Factories/WorkingFamiliesTestScenarioFactory.cs Outdated
Comment thread CheckYourEligibility.API/Gateways/Factories/StandardCheckTestScenarioFactory.cs Outdated

@YiannosGeorgantas YiannosGeorgantas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Lili. The latest changes address the other points I raised. I’m leaving this as request changes for now because the unit tests for the new scenario factory are still outstanding, along with the open clarification about the configured suffixes versus the hard-coded values. I’ll re-review once those are updated.

Comment thread CheckYourEligibility.API/Helpers/WorkingFamiliesCheckHelper.cs
@LiliNedeleva

Copy link
Copy Markdown
Contributor Author

Could we add unit tests around WorkingFamiliesTestScenarioFactory? I can only see the existing helper tests being adjusted, but the new factory introduces the 700–704 scenarios, term-dependent date generation, DVSD and reconfirmation variations, and configurable code types. I think we particularly need coverage for each scenario across the Spring, Summer and Autumn boundaries, the 704 expired path, “cannot be used yet” later in a term, and a gateway test proving that genuine non-test codes still reach ECS/ECE. Injecting the current date and avoiding random dates would also make these tests deterministic.

Add unit tests for WorkingFamiliesTestScenarioFactory and enhance eligibility check mappings + missing enginegateway tests
545f1b3

More integration testing will follow from the consumer side. No more tests will be added to this PR at this stage

// Fallback: generate before the due-now window
wfEvent.ValidityEndDate =
RandomDateGenerator(wfEvent.ValidityStartDate, dueWindowStart.AddDays(-1));
RandomDateGenerator(checkDate.AddDays(1), dueWindowStart.AddDays(-1));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you check this fallback range please? dueWindowStart is assigned from checkDate, so this passes checkDate.AddDays(1) as the start and checkDate.AddDays(-1) as the end. When this fallback is reached near the end of a term, RandomDateGenerator will therefore receive a start date after its end date and throw. What date range should represent the intended “not due now” fallback here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed and amended.

{
Assert.That(
() => _sut.GenerateTestScenarioInternalSide(CreateCheckData("99900000000")),
Throws.TypeOf<NullReferenceException>());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should an unmatched scenario return null rather than make a NullReferenceException part of the expected behaviour? The factory method has a nullable return type, and CheckingEngineGateway explicitly handles a null result by returning notFound. The exception currently occurs because null is passed into PopulateCommonFields, which looks accidental rather than an intentional contract. Could the factory guard against that and this test assert a null result instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, this should be fixed now

[TestCase("70200000000", "AB123456A")]
[TestCase("70300000000", "AB123456A")]
[TestCase("70400000000", "AB123456A")]
public void GenerateTestScenarioInternalSide_GeneratesConfiguredScenario(string eligibilityCode, string nino)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these cases assert the defining outcome of each scenario rather than only that an event and its common fields are returned? The current assertions would still pass if 701, 702, 703 and 704 all invoked the same scenario builder. I think we need assertions confirming that 701 is valid for the current term only, 702 for the current and next terms, 703 has VED passed but GPED still active, and 704 has GPED passed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I have now enhanced the eligibility scenario unit tests.

@YiannosGeorgantas YiannosGeorgantas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the new test coverage, Lili. I’ve identified three points that I think need addressing before approval: the invalid fallback date range, the unmatched-scenario test expecting a NullReferenceException despite the caller supporting a null result, and the need for the scenario tests to verify the distinct outcomes of 701–704. The earlier configuration-suffix conversation also remains open. I’ll re-review once these are updated.

@YiannosGeorgantas

Copy link
Copy Markdown
Contributor

Could we add unit tests around WorkingFamiliesTestScenarioFactory? I can only see the existing helper tests being adjusted, but the new factory introduces the 700–704 scenarios, term-dependent date generation, DVSD and reconfirmation variations, and configurable code types. I think we particularly need coverage for each scenario across the Spring, Summer and Autumn boundaries, the 704 expired path, “cannot be used yet” later in a term, and a gateway test proving that genuine non-test codes still reach ECS/ECE. Injecting the current date and avoiding random dates would also make these tests deterministic.

Add unit tests for WorkingFamiliesTestScenarioFactory and enhance eligibility check mappings + missing enginegateway tests 545f1b3

More integration testing will follow from the consumer side. No more tests will be added to this PR at this stage

Thanks, I’ve reviewed the linked commit and can see the additional factory and gateway coverage. I’ve raised the specific remaining points separately in my latest review, so I’ll continue the re-review through those inline conversations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants