-
Notifications
You must be signed in to change notification settings - Fork 15
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
base: master
Are you sure you want to change the base?
Conversation
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.
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}`), |
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 starting point!
expect(layerCount).toBe(5); // TODO: this is UGA floods specific | ||
expect(iconLayerCount).toBe(5); // TODO: this is UGA floods specific |
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.
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 |
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.
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
.
expect(layerCount).toBe(5); // TODO: this is UGA floods specific | ||
expect(iconLayerCount).toBe(5); // TODO: this is UGA floods specific |
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.
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 |
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.
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) { |
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.
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 |
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.
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 |
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.
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.
Describe your changes
See notes in AB#32771
Initial prototype attempt to incorporate some ideas for multi-disaster-type setup for e2e
Checklist before requesting a review