Skip to content
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

feat(tests): Include report section for when tests fail due to missing opportunity data (random mode) #647

Merged
merged 18 commits into from
Mar 11, 2024

Conversation

lukehesluke
Copy link
Contributor

@lukehesluke lukehesluke commented Feb 27, 2024

closes #643

QA

This zip contains some output summary pages from different runs of Test Suite.

Both runs used config/default-dev.json but with useRandomOpportunities: true.

  • summary_with_no_errors.html is a successful run against just the opportunity-free test. You can see here that there is no "missing data" section
  • summary_with_lots_of_errors.html is a run against the payment category on a Booking System which had no opportunity data (Reference Implementation using OPPORTUNITY_COUNT=1). You can see here the Missing Opportunity Data section

qa.zip

@@ -987,6 +987,7 @@ app.post('/test-interface/datasets/:testDatasetIdentifier/opportunities', functi
logError(`Random Bookable Opportunity from seller ${sellerId} for ${criteriaName} within ${bookingFlow} (${opportunityType}) call failed: No matching opportunities found`);
res.status(404).json({
error: `Opportunity of type '${opportunityType}' that has an \`organizer\`/\`provider\` with \`@id\` '${sellerId}', that also matched '${criteriaName}' within '${bookingFlow}', was not found within the feeds.`,
type: 'OpportunityNotFound',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that this 404 is not confused with other types of 404s when determining if there are missing opportunities

Comment on lines +465 to +477
const opportunityNotFound = (
respObj.response.statusCode === 404
&& respObj.body?.type === 'OpportunityNotFound'
);
if (opportunityNotFound) {
this.logger.recordFlowStageEvent(stage, {
type: 'OpportunityNotFound',
opportunityType,
testOpportunityCriteria,
bookingFlow,
sellerId,
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where the "opportunity missing" event is saved

{{#each missingOpportunityDataSummary}}

<li>
❌️ Missing Criteria: <strong><a href="https://openactive.io/test-interface#{{@key}}">{{@key}}</a></strong>
Copy link
Contributor Author

@lukehesluke lukehesluke Feb 28, 2024

Choose a reason for hiding this comment

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

Lots of ❌️s to hopefully make it clear that these are important issues 🤷

@lukehesluke lukehesluke marked this pull request as ready for review February 28, 2024 10:41
@lukehesluke lukehesluke requested review from civsiv and nickevansuk and removed request for civsiv February 28, 2024 10:41
@@ -1,3 +1,27 @@
// TODO fix these issues!
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a good issue to raise and do, for maintainability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point @civsiv . I've created an issue to do that, and broadly fix all the ESLint sins, here: #648

Copy link
Collaborator

@nickevansuk nickevansuk left a comment

Choose a reason for hiding this comment

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

Brilliantly elegant solution 👍 - a couple of wording suggestions and something about grouping

Comment on lines 14 to 30
Some of the tests failed because there were not enough opportunities matching
the required [criteria](https://github.com/openactive/openactive-test-suite/blob/master/packages/openactive-integration-tests/test/features/README.md).

This only happens when using [Random Mode](https://developer.openactive.io/open-booking-api/test-suite#random-mode)
as opposed to [Controlled Mode](https://developer.openactive.io/open-booking-api/test-suite#controlled-mode).

In order to pass these tests, you will need to generate more opportunities in
your Booking System. Make sure that there are many opportunities available for
each criteria.

You can use [test-data-generator](https://github.com/openactive/openactive-test-suite/tree/master/packages/openactive-integration-tests/test-data-generator)
to get a specification of how much of each type of opportunity you need to
generate.

If any of these criteria are not relevant to your Booking System, you may
need to remove the relevant feature from your Test Suite configuration
(See [`implementedFeatures`](https://github.com/openactive/openactive-test-suite/tree/master/packages/openactive-integration-tests#implementedfeatures)).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Some of the tests failed because there were not enough opportunities matching
the required [criteria](https://github.com/openactive/openactive-test-suite/blob/master/packages/openactive-integration-tests/test/features/README.md).
This only happens when using [Random Mode](https://developer.openactive.io/open-booking-api/test-suite#random-mode)
as opposed to [Controlled Mode](https://developer.openactive.io/open-booking-api/test-suite#controlled-mode).
In order to pass these tests, you will need to generate more opportunities in
your Booking System. Make sure that there are many opportunities available for
each criteria.
You can use [test-data-generator](https://github.com/openactive/openactive-test-suite/tree/master/packages/openactive-integration-tests/test-data-generator)
to get a specification of how much of each type of opportunity you need to
generate.
If any of these criteria are not relevant to your Booking System, you may
need to remove the relevant feature from your Test Suite configuration
(See [`implementedFeatures`](https://github.com/openactive/openactive-test-suite/tree/master/packages/openactive-integration-tests#implementedfeatures)).
Some of the tests failed because there were not enough opportunities in the booking system under test that matched the required [criteria](https://github.com/openactive/openactive-test-suite/blob/master/packages/openactive-integration-tests/test/features/README.md).
This only happens when using [Random Mode](https://developer.openactive.io/open-booking-api/test-suite#random-mode)
as opposed to [Controlled Mode](https://developer.openactive.io/open-booking-api/test-suite#controlled-mode).
In order to pass these tests, please increase the number of opportunities in the Booking System. Make sure that there are many opportunities available that match each criteria.
You can use [test-data-generator](https://github.com/openactive/openactive-test-suite/tree/master/packages/openactive-integration-tests/test-data-generator)
to get a specification of how many of each type of opportunity needs to exist in the Booking System.
If any of these criteria are not relevant to your Booking System, you may
need to remove the relevant feature from your Test Suite configuration
(See [`implementedFeatures`](https://github.com/openactive/openactive-test-suite/tree/master/packages/openactive-integration-tests#implementedfeatures)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've implemented this

Comment on lines +456 to +457
* Groups the results from `missingOpportunityDataStats` into something
* that can be shown in the summary report.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest that the count of opportunities is displayed for each combination of Sellers, Opportunity Types and Booking Flows.

For example, looking at the error below, it is not clear what opportunities need to be created (how many ScheduledSessions and how many Slots across the 72):
Screenshot 2024-03-04 at 14 45 33

Copy link
Contributor Author

@lukehesluke lukehesluke Mar 5, 2024

Choose a reason for hiding this comment

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

@nickevansuk this is similar to my initial implementation for this, but the issue was that the output would very quickly become huge. Even just for the payment category of tests (let alone the full test run!), the missing data section was several pages long and which is just number counts for each combination, which one can easily get lost in, and we also now have the reverse problem of users not realising that there are comprehensive test results at the end of all this!

I had a chat with @civsiv about this as well and we considered a few possible designs and then agreed that it would be good to get this information across, but difficult at present. And, regardless, it's currently possible to get this information by running Test Data Generator (which is now recommended in the output text if there is any missing data).

Besides that, the main use case which we're aware of from our experience is when a data publisher has simply forgotten to run some script or do some action which generates loads of random test data. So in those sorts of cases, they just need a rough idea that they need to generate more data.

In future, perhaps we could have something like the summarised view as it is but which links to a more complicated file, which perhaps is just generated using test-data-generator (DRY) to shortcut the journey for the user.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great thinking! Responding to this late via a new issue here: #684

Comment on lines +34 to +35
This is a list of the criteria, opportunity type, etc for which no opportunities
were found, that also includes a list of which tests failed as a result:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This is a list of the criteria, opportunity type, etc for which no opportunities
were found, that also includes a list of which tests failed as a result:
This is a list of the opportunities that the test suite requires which were not found in the Booking System:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(see my next comment)

{{#each missingOpportunityDataSummary}}

<li>
❌️ Missing Criteria: <strong><a href="https://openactive.io/test-interface#{{@key}}">{{@key}}</a></strong>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
❌️ Missing Criteria: <strong><a href="https://openactive.io/test-interface#{{@key}}">{{@key}}</a></strong>
❌️ Missing opportunities matching criteria: <strong><a href="https://openactive.io/test-interface#{{@key}}">{{@key}}</a></strong>

As it's the opportunities that are missing rather than the criteria?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's opportunities that are missing but the list is grouped by Criteria. Which was arrived at in the summarization process that I talked about in my earlier comment

@lukehesluke lukehesluke force-pushed the feature/report-random-mode-ran-out-of-data branch from 170d0be to 4da1972 Compare March 5, 2024 14:22
@lukehesluke lukehesluke merged commit e2f4329 into master Mar 11, 2024
32 checks passed
@lukehesluke lukehesluke deleted the feature/report-random-mode-ran-out-of-data branch March 11, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants