Create Gherkin tests for Population Density Data by jgarciahospital · Pull Request #44 · camaraproject/PopulationDensityData

jgarciahospital · 2024-08-02T11:27:23Z

What type of PR is this?

Add one of the following kinds:

tests

What this PR does / why we need it:

Including test proposal for population density data 0.1.1

Which issue(s) this PR fixes:

Fixes #37

bigludo7

See few comments for the test Definition.

Not sure why the README.md & checklist part of this PR.

bigludo7 · 2024-08-28T12:13:28Z

code/Test_definitions/population-density-data.feature

+        And the response property "$.status" value is "SUPPORTED_AREA"
+        And the response property "$.timedPopulationDensityData[*].startTime" is equal to or later than request body property "$.startDate"
+        And the response property "$.timedPopulationDensityData[*].endTime" is equal to or earlier than request body property "$.endDate"
+        And the response property "$.timedPopulationDensityData[*].cellPopulationDensityData[*].populationDensityData.dataType.datatype" is equal to "LOW_DENSITY" or "DENSITY_ESTIMATION"


duplicate datatype no?
should be "$.timedPopulationDensityData[*].cellPopulationDensityData[*].populationDensityData.dataType" is equal to "LOW_DENSITY" or "DENSITY_ESTIMATION"

to be solved

bigludo7 · 2024-08-28T12:18:38Z

code/Test_definitions/population-density-data.feature

+        And the response property "$.status" value is "SUPPORTED_AREA"
+        And the response property "$.timedPopulationDensityData[*].startTime" is equal to or later than request body property "$.startDate"
+        And the response property "$.timedPopulationDensityData[*].endTime" is equal to or earlier than request body property "$.endDate"
+        And the response property "$.timedPopulationDensityData[*].cellPopulationDensityData[*].populationDensityData.dataType.datatype" is equal to "LOW_DENSITY" or "DENSITY_ESTIMATION"


should we add that
"$.timedPopulationDensityData[*].cellPopulationDensityData[*].populationDensityData.maxPplDensity must be filled?
same for minPplDensity and pplDensity

to be solved

bigludo7 · 2024-08-28T12:20:30Z

code/Test_definitions/population-density-data.feature

+        And the response property "$.status" value is "PART_OF_AREA_NOT_SUPPORTED"
+        And the response property "$.timedPopulationDensityData[*].startTime" is equal to or later than request body property "$.startDate"
+        And the response property "$.timedPopulationDensityData[*].endTime" is equal to or earlier than request body property "$.startDate"
+        And there is at least one item in response property "$.timedPopulationDensityData[*].cellPopulationDensityData[*].datatype" equal to "NO_DATA"


and add:
And there is at least one item in response property "$.timedPopulationDensityData[*].cellPopulationDensityData[*].datatype" equal to "LOW_DENSITY" or "DENSITY_ESTIMATION"

to be solved

@jgarciahospital : is it solved ?

For me yes :) line 61

Thanks ludovic, solved then.

bigludo7 · 2024-08-28T12:24:15Z

code/Test_definitions/population-density-data.feature

+        And the response property "$.status" value is "AREA_NOT_SUPPORTED"
+        And the response property "$.timedPopulationDensityData" is an empty array
+
+    @population_density_data_04_webhook_success_scenario


I have an issue on this one because we are not aligned with commonalities for the webjook. It should be sink & sinkCredential. See commonalities or carrier billing API where the pattern has been aligned. I will post an issue.

If you consider that it's tool ate to align not sure keeping the test worthwhile.

and here we have the tricky one. This also requires changes in the API yaml, but APi is now in the public release phase. @bigludo7 let me propose something, please confirm:

we create a new PR to update API yaml

we update this .feature also with this and other changes

updates are reported in the Prepare release r1.2 #45

Hello @jgarciahospital
This is tricky yes.
@rartych provided another view: as this API did not use strictly the explicit subscription model, this is is not an issue for him. So perhaps we can change....nothing here for this version (and keep this test) and we aligne for next release.
WDYT? I'm a bit nervous to have all these change...

@rartych any proposal about this? Just to ensure that we close it before review. No problem with any of the options, I think we can provide both code proposals now.

I would stay with the current definition and discuss changes for version 0.2 of the API. In fact we don't have here Event Notification defined in https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#12-subscription-notification--event , but asynchronous response. There are also no explicit guidelines for that scenario in Commonalities.

Ok, let me then update gherkin with only the rest of small comments coming from @bigludo7, and also main yaml with some only editorials, and we are ready for closing the release.

Please @sachinvodafone confirm after that review for been able to merge the test plan.

Sorry, making one additional review we find that QoD API suffered similar issue and finally they made the alignement (this Issue & PR summarizes). So, just for aligning with https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#instance-based-implicit-subscription, let me propose to roll back to initial plan and:

Update gherkin to be aligned with async auth

Update API yaml (I'll create a separate PR moving back to wip version)

Later we can sync this with the release PR

jgarciahospital · 2024-09-04T07:24:24Z

See few comments for the test Definition.

Not sure why the README.md & checklist part of this PR.

Thanks Ludo for the review!

readme.md is just to delete the readme of the test definitions folder, now that we have another file.
Checklist is just to include the test reference.

Including small typos correction and alignement of "$.sink" and "$.sinkCredentials" properties.

bigludo7

Sorry still one comment but this one break the file if not resolved.

bigludo7 · 2024-09-05T17:00:48Z

code/Test_definitions/population-density-data.feature

+        And the request property "$.sinkCredentials.credentialType" is set to "ACCESSTOKEN"
+        And the request property "$.sinkCredentials.accessTokenType" is set to "bearer"
+        And the request property "$.sinkCredentials.accessToken" is set to a valid access token accepted by the events receiver
+        And the request property "$.sinkCredentials.accessTokenExpiresUtc" is set to a value long enough in the future        When the request "retrievePopulationDensity" is sent


Typo here - When the request....must be on the next line

bigludo7

LGTM

jgarciahospital · 2024-09-06T10:15:20Z

Same here, thanks @bigludo7 . @sachinvodafone please for your validation.

tanjadegroot · 2024-09-06T17:02:51Z

code/Test_definitions/population-density-data.feature

+
+    Background: Common retrievePopulationDensity  setup
+        Given an environment at "apiRoot"
+        And the resource "/population-density-data/v1/retrieve"


should be v0.1 in the url

tanjadegroot · 2024-09-06T17:16:05Z

documentation/API_documentation/Population-Density-Data-API-Readiness-Checklist.md

Recommendation: The filename should be population-density-data-API-Readiness-Checklist.md
Links to this file should then be updated as well, if any (e.g. on API release tracker)

Solved, to be spotted in #45 to ensure it is maintained when merging.

Fix API version

…on-density-data-API-Readiness-Checklist.md

tanjadegroot

It looks good to me except for the statement that still needs to be resolved. Should it be resolved ? if so please do and then for this PR: LGTM from RM

tanjadegroot · 2024-09-10T14:39:17Z

code/Test_definitions/population-density-data.feature

+        And the response property "$.status" value is "PART_OF_AREA_NOT_SUPPORTED"
+        And the response property "$.timedPopulationDensityData[*].startTime" is equal to or later than request body property "$.startDate"
+        And the response property "$.timedPopulationDensityData[*].endTime" is equal to or earlier than request body property "$.startDate"
+        And there is at least one item in response property "$.timedPopulationDensityData[*].cellPopulationDensityData[*].datatype" equal to "NO_DATA"


@jgarciahospital : is it solved ?

tanjadegroot · 2024-09-11T11:00:59Z

Ludovic's comments:
https://github.com/camaraproject/PopulationDensityData/pull/44/files/6e74f3edccc5fffc682d19fc2c530b71c8fc9871#r1734566694
and
https://github.com/camaraproject/PopulationDensityData/pull/44/files/6e74f3edccc5fffc682d19fc2c530b71c8fc9871#r1734573307
are still open

@jgarciahospital @maheshc01 @sachinvodafone please provide your view ASAP as we would like to close the M4 release today if possible.

Create population-density-data.feature

3f3e232

jgarciahospital added Fall24 RC2 labels Aug 2, 2024

jgarciahospital requested review from maheshc01 and sachinvodafone as code owners August 2, 2024 11:27

jgarciahospital added 3 commits August 2, 2024 13:29

Delete code/Test_definitions/README.md

dd3221c

Typo fixing

41cb40e

Updating Checklist with ATP link

6e74f3e

jgarciahospital mentioned this pull request Aug 23, 2024

Prepare release r1.2 #45

Merged

bigludo7 reviewed Aug 28, 2024

View reviewed changes

hdamker requested a review from bigludo7 September 4, 2024 08:03

Update population-density-data.feature

6537942

Including small typos correction and alignement of "$.sink" and "$.sinkCredentials" properties.

jgarciahospital mentioned this pull request Sep 5, 2024

Validate API code and align with commonalities (meta-release item 1-2-3) #38

Closed

bigludo7 reviewed Sep 5, 2024

View reviewed changes

typo correction

33fea08

bigludo7 approved these changes Sep 6, 2024

View reviewed changes

tanjadegroot requested changes Sep 6, 2024

View reviewed changes

jgarciahospital added 2 commits September 6, 2024 20:58

Update population-density-data.feature

772156c

Fix API version

Rename Population-Density-Data-API-Readiness-Checklist.md to populati…

888e853

…on-density-data-API-Readiness-Checklist.md

jgarciahospital requested a review from tanjadegroot September 10, 2024 10:11

tanjadegroot reviewed Sep 10, 2024

View reviewed changes

sachinvodafone approved these changes Sep 11, 2024

View reviewed changes

jgarciahospital merged commit 79c89d7 into main Sep 11, 2024

jgarciahospital deleted the jgarciahospital-patch-4 branch September 11, 2024 12:07

Conversation

jgarciahospital commented Aug 2, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Uh oh!

bigludo7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanjadegroot Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigludo7 Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgarciahospital commented Sep 4, 2024

Uh oh!

bigludo7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bigludo7 left a comment

Choose a reason for hiding this comment

Uh oh!

jgarciahospital commented Sep 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanjadegroot left a comment

Choose a reason for hiding this comment

Uh oh!

tanjadegroot Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanjadegroot commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

tanjadegroot Sep 10, 2024 •

edited

Loading

bigludo7 Sep 10, 2024 •

edited

Loading

tanjadegroot Sep 10, 2024 •

edited

Loading

tanjadegroot commented Sep 11, 2024 •

edited

Loading