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

prototype: e2e multi-disaster-type setup AB#32771 #1934

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jannisvisser
Copy link
Contributor

@jannisvisser jannisvisser commented Jan 22, 2025

Describe your changes

See notes in AB#32771

Initial prototype attempt to incorporate some ideas for multi-disaster-type setup for e2e

  • See 'ScenarioNoTrigger.spec' only for now
  • within it I loop over disaster-types
  • there's an example of a test that is skipped conditional on disaster-type
  • there's an example of an assertion within a test being skipped conditional on disaster-type

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added tests wherever relevant
  • I have made sure that all automated checks pass before requesting a review

@jannisvisser jannisvisser marked this pull request as draft January 22, 2025 15:29
Copy link
Contributor

@Piotrk39 Piotrk39 left a comment

Choose a reason for hiding this comment

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

It looks good to me. I think I understand the concept. I just wonder if we still want to reuse the same 2 tests trigger and no trigger. Maybe we want to extend it with other scenarios e.g trigger-drought etc.? Or we want to loop through the reset endpoints and execute the same scenarios?


export default (
pages: Partial<Pages>,
components: Partial<Components>,
disasterType: string,
) => {
test(
qase(2, 'Map component elements should be visible'),
qase(2, `Map component elements should be visible - ${disasterType}`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Good starting point!

Comment on lines +78 to +79
expect(layerCount).toBe(5); // TODO: this is UGA floods specific
expect(iconLayerCount).toBe(5); // TODO: this is UGA floods specific
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can change it to > 0

@@ -73,17 +73,18 @@ class AggregatesComponent extends DashboardPage {
const iconLayerCount = await this.aggregatesInfoIcon.count();

// Basic Assertions
expect(headerTextModified).toBe('National View 0 Predicted Flood(s)');
expect(headerTextModified).toBe('National View 0 Predicted Drought(s)'); // TODO: make Flood(s) disaster-type agnostic
Copy link
Member

Choose a reason for hiding this comment

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

To make this hazard agnostic, the test should assert the result of getAggregateHeader. In a black-box approach so we only check for the final text.

An even simpler approach is to simple make the expected value a part of testData.

Comment on lines +78 to +79
expect(layerCount).toBe(5); // TODO: this is UGA floods specific
expect(iconLayerCount).toBe(5); // TODO: this is UGA floods specific
Copy link
Member

Choose a reason for hiding this comment

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

This test depends on how we interpret the map layer feature.

We can decide that all flood portals will have 5 layers and test for the specific number. In this case, the expected values should be added to testData.

An alternative way is to calculate based on scenario configs which will also become a part of testData.

The former approach makes sense to me in the short term but the latter seems scalable (if that's what we want for our test suites).


// Loop through the layers and check if they are present with correct data
for (const affectedNumber of affectedNumbers) {
const affectedNumberText = await affectedNumber.textContent();
expect(affectedNumberText).toContain('0');
}
// Loop through the layers and check if they are present with correct names
// TODO: expectedLayersNames is UGA floods specific
Copy link
Member

Choose a reason for hiding this comment

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

This adds weight to the scenario configs suggested in my previous comment. The expected layer names should probably come from testData.

await map.breadCrumbViewIsVisible({ nationalView: true });

// PROTOTYPE EXAMPLE of disaster-type specific assertion
if (disasterType === DisasterTypeEnum.Floods) {
Copy link
Member

Choose a reason for hiding this comment

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

Are the breadcrumbs only used in flood?

We should use these features consistently for all country hazards. We can use conditional tests as a crutch until we refactor these exceptions out of the portal. In the long term, such branching of logic will only get more complex thus making IBF (and its tests) unmanageable.

@@ -25,6 +26,12 @@ export default (
throw new Error('pages and components not found');
}

// PROTOTYPE EXAMPLE of disaster-type specific test skip
Copy link
Member

Choose a reason for hiding this comment

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

This should be conditional based on whether glofas_stations layer is in the scenario's testData. So it's not tied to a specific hazard.

My interpretation is the visibility of the glofas_stations layer is not tied to a specific hazard. It is tied according to the data but not as the feature's behaviour. The feature is implemented in a way the glofas_stations will be visible regardless of the hazard as long as the layer is enabled (in the database) and data is available. An extreme example is that the glofas_stations layer is enabled for any hazard including drought.

The above assumes we want the tests to test a feature and not the data flowing through the feature. It's difficult to make this distinction, especially in e2e tests.

If we stick to these conditional tests, I prefer that the conditions be checked outside the test. So the check will be done here, for example

if (disasterType === DisasterTypeEnum.Floods) {
    MapComponentGloFASStations(pages, components, disasterType); 
    MapComponentGloFASStationsWarning(pages, components, disasterType); 
}

@@ -31,7 +31,7 @@ export default (
await userState.headerComponentIsVisible({
countryName: NoTriggerDataSet.CountryName,
});
await timeline.timelineIsInactive();
await timeline.timelineIsInactive(); // TODO: make conditional on disaster-type
Copy link
Member

Choose a reason for hiding this comment

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

Are the breadcrumbs timelines inactive only used in flood?

We should use these features consistently for all country hazards. We can use conditional tests as a crutch until we refactor these exceptions out of the portal. In the long term, such branching of logic will only get more complex thus making IBF (and its tests) unmanageable.

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