Wip for M4

[FabrizioMoggio]

What type of PR is this?

• cleanup
• documentation

What this PR does / why we need it:

to fix the new issues post M3 to meet M4

Which issue(s) this PR fixes

#20

[ToshiWakayama-KDDI]

Hi @FabrizioMoggio ,

Thanks very much for this PR. Could you look after the .feature file as well?

Many thanks,
Toshi

[FabrizioMoggio]

Hi @FabrizioMoggio ,

Thanks very much for this PR. Could you look after the .feature file as well?

Many thanks, Toshi

sure, I can even prepare the public release when this wip will be approved and merged. Just one question, what is missing in the .feature files?

[ToshiWakayama-KDDI]

Thanks, @FabrizioMoggio . Regarding the .feature file, it also has version numbers, which should be reset to vwip. We could refer to camaraproject/Tenure#46 .

Thanks,
Toshi

[FabrizioMoggio]

Thanks, @FabrizioMoggio . Regarding the .feature file, it also has version numbers, which should be reset to vwip. We could refer to camaraproject/Tenure#46 .

Thanks, Toshi

I usually rollback to wip only the files that have been changed, anyway I have done it :-)

[FabrizioMoggio]

I checked the novelties in Commonalities here: #29

In my understanding, there was just one fix to do: #29 (comment)

[fernandopradocabrillo]

LGTM! thanks @FabrizioMoggio

[FabrizioMoggio]

we have a problem with the .feature file:

0      Number of scenarios exceeds maximum: 101/50          max-scenarios-per-file
177    Element Scenario too long: actual 27, expected 15    scenario-size

how can we fix this?

0: for the number of scenarios, we could split the .feature file in two, one for sunny day scenarios and one for errors.

177: this is for scenario KYC_Match_6_: it is too long. Can we split it? Or maybe we can reduce the number of optional tested parameters. To avoid errors from my side, can you please email me the code for the new KYC_Match_6_?

[ToshiWakayama-KDDI]

Thanks, @FabrizioMoggio . This seems heavy. I would like to see Fernando's view.

Best regards,
Toshi

[ToshiWakayama-KDDI]

Hi @FabrizioMoggio ,
Unfortunately, it seems Fernando is on holiday until 28th Friday.

Hi @hdamker ,
Maybe this is a silly question, but are there restrictions really existing for the .feature file errors as below?

• Number of scenarios exceeds maximum: 101/50 max-scenarios-per-file
• Element Scenario too long: actual 27, expected 15 scenario-size

Many thanks,
Toshi
KDDI

[ToshiWakayama-KDDI]

BTW @FabrizioMoggio , @fernandopradocabrillo ,

Regarding the ErrorInfo order, Pedro Diez created PR #517 ( camaraproject/Commonalities#517 ) to correct the order in the ErrorInfo order in the CAMARA_common.yaml (from Message->Status->Code to Status->Code->Message). So, we don't need to have this change included in this PR.

Could you correct this PR (for yaml), please, @FabrizioMoggio ?

Many thanks,
Toshi
KDDI

[FabrizioMoggio]

BTW @FabrizioMoggio , @fernandopradocabrillo ,

Regarding the ErrorInfo order, Pedro Diez created PR #517 ( camaraproject/Commonalities#517 ) to correct the order in the ErrorInfo order in the CAMARA_common.yaml (from Message->Status->Code to Status->Code->Message). So, we don't need to have this change included in this PR.

Could you correct this PR (for yaml), please, @FabrizioMoggio ?

Many thanks, Toshi KDDI

Dear @ToshiWakayama-KDDI , we had already agreed on that so that change has been already implemented (b73958c). Unless I got your request wrong :-)

[hdamker]

Hi @hdamker , Maybe this is a silly question, but are there restrictions really existing for the .feature file errors as below?

• Number of scenarios exceeds maximum: 101/50 max-scenarios-per-file
• Element Scenario too long: actual 27, expected 15 scenario-size

@FabrizioMoggio These rules are defined in Commonalities (cf. artifacts/linting_rules/.gherkin-lintrc) and I can't judge if they are absolutely needed as I'm not an expert for these.

In your case the counting of scenarios has expanded the scenario outlines, which hit the rule
"max-scenarios-per-file": ["on", {"maxScenarios": 50, "countOutlineExamples": true}],
Please raise this as a question in Commonalities. Until you have an answer I would tend to ignore this one as the file seems to be readable and maintainable.

Regarding the second one, here a proposal generated by Copilot, grouping all optional parameters into a single step, which is still below the length limit (of 190):

@KYC_Match_6_success_multiple_optional_parameter_combinations
Scenario: Validate success response when providing different optional parameter combinations
    Given a valid testing phone number supported by the service, identified by the access token or provided in the request body
    And the request body contains valid required properties
    And the request body contains a random combination of valid optional parameters, including identity, name, address, birthdate, email, gender, and nationality fields
    When the request "KYC_Match" is sent
    Then the response status code is 200
    And the response header "x-correlator" has same value as the request header "x-correlator"
    And the response header "Content-Type" is "application/json"
    And the response body complies with the OAS schema at "/components/schemas/KYC_MatchResponse" 

[FabrizioMoggio]

Hi @hdamker , Maybe this is a silly question, but are there restrictions really existing for the .feature file errors as below?

• Number of scenarios exceeds maximum: 101/50 max-scenarios-per-file
• Element Scenario too long: actual 27, expected 15 scenario-size

@FabrizioMoggio These rules are defined in Commonalities (cf. artifacts/linting_rules/.gherkin-lintrc) and I can't judge if they are absolutely needed as I'm not an expert for these.

In your case the counting of scenarios has expanded the scenario outlines, which hit the rule "max-scenarios-per-file": ["on", {"maxScenarios": 50, "countOutlineExamples": true}], Please raise this as a question in Commonalities. Until you have an answer I would tend to ignore this one as the file seems to be readable and maintainable.

Regarding the second one, here a proposal generated by Copilot, grouping all optional parameters into a single step, which is still below the length limit (of 190):

@KYC_Match_6_success_multiple_optional_parameter_combinations
Scenario: Validate success response when providing different optional parameter combinations
    Given a valid testing phone number supported by the service, identified by the access token or provided in the request body
    And the request body contains valid required properties
    And the request body contains a random combination of valid optional parameters, including identity, name, address, birthdate, email, gender, and nationality fields
    When the request "KYC_Match" is sent
    Then the response status code is 200
    And the response header "x-correlator" has same value as the request header "x-correlator"
    And the response header "Content-Type" is "application/json"
    And the response body complies with the OAS schema at "/components/schemas/KYC_MatchResponse" 

@hdamker Thank you very much for the suggestions.

@ToshiWakayama-KDDI , @fernandopradocabrillo , @GillesInnov35 if you agree on the solution proposed by Copilot on 177, then we can consider my proposal for 0:

0: split the file in two for "sunny days" and "rainy days". these would be an easy fix
7: reduce the number of the lines as suggested by Copilot

This should let us pass the test.

Anyway, I'm going to open a request in Commonalities.

[FabrizioMoggio]

Discussion in Commonalities: camaraproject/Commonalities#519

[ToshiWakayama-KDDI]

BTW @FabrizioMoggio , @fernandopradocabrillo ,
Regarding the ErrorInfo order, Pedro Diez created PR #517 ( camaraproject/Commonalities#517 ) to correct the order in the ErrorInfo order in the CAMARA_common.yaml (from Message->Status->Code to Status->Code->Message). So, we don't need to have this change included in this PR.
Could you correct this PR (for yaml), please, @FabrizioMoggio ?
Many thanks, Toshi KDDI

Dear @ToshiWakayama-KDDI , we had already agreed on that so that change has been already implemented (b73958c). Unless I got your request wrong :-)

Oh, I did not realise it. Thank you so much, @FabrizioMoggio !

BR
Toshi

[ToshiWakayama-KDDI]

Hi @hdamker , @FabrizioMoggio ,

Regarding the first one,

@FabrizioMoggio These rules are defined in Commonalities (cf. artifacts/linting_rules/.gherkin-lintrc) and I can't judge if they are absolutely needed as I'm not an expert for these.

In your case the counting of scenarios has expanded the scenario outlines, which hit the rule
"max-scenarios-per-file": ["on", {"maxScenarios": 50, "countOutlineExamples": true}],
Please raise this as a question in Commonalities. Until you have an answer I would tend to ignore this one as the file seems to be readable and maintainable.

Thank you @hdamker . That makes much sense. I have just checked the kyc-match .feature file, and found the following:

• @KYC_Match_2_success_specific_property Scenario Outline: 24 example parameters
• @KYC_Match_3_success_specific_property_score Scenario Outline: 14 example parameters
• @KYC_Match_4_perfect_match_no_scores_returned Scenario Outline: 23 example parameters
• @KYC_Match_5_success_specific_property_false Scenario Outline: 23 example parameters
• Others are Scenarios, and there are 17 Scenarios altogether

->24+14+23+23+17=101, same as the number of scenarios the error shows.

Now we understand how the error was raised, and @FabrizioMoggio has raised the question in Commonalities, and @hdamker has kindly offered 'Until you have an answer I would tend to ignore this one as the file seems to be readable and maintainable.', so, why don't we leave this as it is (not to split Sunny scenarios and Rainy scenarios) for a while?

Best regards,
Toshi

[ToshiWakayama-KDDI]

Hi @FabrizioMoggio , @hdamker ,

Regarding the second one,

@KYC_Match_6_success_multiple_optional_parameter_combinations
Scenario: Validate success response when providing different optional parameter combinations
    Given a valid testing phone number supported by the service, identified by the access token or provided in the request body
    And the request body contains valid required properties
    And the request body contains a random combination of valid optional parameters, including identity, name, address, birthdate, email, gender, and nationality fields
    When the request "KYC_Match" is sent
    Then the response status code is 200
    And the response header "x-correlator" has same value as the request header "x-correlator"
    And the response header "Content-Type" is "application/json"
    And the response body complies with the OAS schema at "/components/schemas/KYC_MatchResponse" 

Thank you so much for the suggestion! Generally it looks very good. One concern I have is that including identity, name, address, birthdate, email, gender, and nationality fields does not include all the parameters that the original KYC_Match_6 scenario has.
Could the line (2nd And) be modified to And the request body contains a random combination of valid optional parameters, including idDocument, name, givenName, familyName, nameKanaHankaku, nameKanaZenkaku, middleNames, familyNameAtBirth, address, streetName, streetNumber, postalCode, region, locality, country, houseNumberExtension, birthdate, email, gender, nationality fields ? (I just wrote down all the paramters.)
Or, will this modification cause another error?

If this modification is ok, my concern will go away, and let's go for this solution.

I will check the Commonalities rules, but if you know the answer, please let me shortcut.

Many thanks,
Toshi

[FabrizioMoggio]

And the request body contains a random combination of valid optional parameters, including idDocument, name, givenName, familyName, nameKanaHankaku, nameKanaZenkaku, middleNames, familyNameAtBirth, address, streetName, streetNumber, postalCode, region, locality, country, houseNumberExtension, birthdate, email, gender, nationality fields

@ToshiWakayama-KDDI we can try, the only doubt is that, for what I remember, there also is a maximum line length.

[FabrizioMoggio]

As I expected we got:

"180: Step name is too long. Length of 334 is longer than the maximum allowed: 190"

[FabrizioMoggio]

We could reduce the line providing just an example for the possible returned parameters:

And the request body contains a random combination of valid optional parameters, including, but not limited to, idDocument, name, givenName, familyName, address, region, , email, gender, nationality

[ToshiWakayama-KDDI]

Thanks, @FabrizioMoggio . 190 is the limit for a line. The pharse 'including, but not limited to,' is fine with me. Could you please make the line accordingly within the 190 limit.

Thanks.

[ToshiWakayama-KDDI]

Thanks, @FabrizioMoggio . Sorry for the delay. It was 10pm in Japan when I finished Fill-in and Age Verification, so, I had to leave the office and come home.

Now I am at home. I have just approved the PR, and it seems I could merge the PR. Is it OK to merge the PR, even though there is 1 failing from the workflow? > @hdamker

Thanks.
Toshi

[ToshiWakayama-KDDI]

Hi @FabrizioMoggio ,

Sorry for the delay. I had quick dinner.

Let me try to merge the PR, although Herbert has not replied yet, but the error is the known Number of scenarios exceeds maximum: 101/50 max-scenarios-per-file and Herbert previously mentioned 'until you have an answer I would tend to ignore this one as the file seems to be readable and maintainable.'

Thanks.

[ToshiWakayama-KDDI]

Good. The PR has successfuly been merged. Could you preceed with the M4 Release PR, @FabrizioMoggio ?

Thanks,