Skip to content

Mav 3408#1000

Open
chinmaymudholkar1 wants to merge 4 commits intomainfrom
mav-3408
Open

Mav 3408#1000
chinmaymudholkar1 wants to merge 4 commits intomainfrom
mav-3408

Conversation

@chinmaymudholkar1
Copy link
Collaborator

Added test for https://nhsd-jira.digital.nhs.uk/browse/MAV-3408 with multiple duplicate vaccs records and one unique record.

@chinmaymudholkar1 chinmaymudholkar1 requested a review from a team as a code owner March 11, 2026 08:49
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why has this fixture been put here? Usually we try to keep all the fixtures in mavis.test.fixtures.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a test fixture and is used by tests in two modules:
test_import_offline_vaccinations.py
test_national_reporting.py

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that makes sense. To avoid the conftest.py file getting too big, we've put shared fixtures in the mavis.test.fixtures module so they are better organised.

Here's an example one for national reporting:

@pytest.fixture
def national_reporting_file_generator(
national_reporting_organisation,
schools,
national_reporting_nurse,
children,
year_groups,
):
return FileGenerator(
national_reporting_organisation,
schools,
national_reporting_nurse,
children,
None,
year_groups,
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure data_models.py is the best place for this fixture. The fixtures there deal with data, not pages and navigation. Maybe helpers.py would be better, or some other place (perhaps even a new file).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what I had thought and created conftest.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in principle the fact that you created a new file wasn't a problem, but that it wasn't in the fixtures/ directory. Though looking at what else is in fixtures/helpers.py, I think it would be fine to simply put it there.

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.

3 participants