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

Create API-Testing-Guidelines.md #117

Merged

Conversation

shilpa-padgaonkar
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • documentation

What this PR does / why we need it:

Adds API-testing-guidelines doc

Which issue(s) this PR fixes:

Fixes #94

Special notes for reviewers:

Changelog input

 release-note

Additional documentation

This section can be blank.

docs

@shilpa-padgaonkar
Copy link
Collaborator Author

@bigludo7 @jlurien @mdomale: Kindly review if all your points have been covered in the doc.

@mdomale
Copy link

mdomale commented Jan 9, 2024

@shilpa-padgaonkar Document looks fine to me.

Copy link
Collaborator

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

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

/LGTM
Maybe a link to subproject sample could help? This one for exemple
Submission of test cases by trehman-gsma · Pull Request #68 · camaraproject/SimSwap (github.com

@shilpa-padgaonkar
Copy link
Collaborator Author

@patrice-conil : made the fix as you suggested. I simply used qod as example because simswap example is still a PR

bigludo7 added a commit to camaraproject/DeviceLocation that referenced this pull request Jan 12, 2024
@bigludo7
Copy link
Collaborator

Hello @shilpa-padgaonkar
First, thanks for the effort :)

Like my colleague @patrice-conil I think referring a sample is very useful.

I have an issue with QoD file because it not reflect the rules (several when/then, name). I've applied rules here: camaraproject/DeviceLocation#119 (review) - Could you please take a look on this proposal and check if it aligned with your view?

We should have an exemple fully aligned with these rules i guess and use it as base.

@shilpa-padgaonkar
Copy link
Collaborator Author

@thanks Ludovic for your quick feedback. I simply added qod as an example for the location (where should the feature file reside).
But you are right, a link to a sample which is based on these guidelines would be a very good add. I have looked at your feature file and it LGTM. I will also request @mdomale to briefly take a look and provide quick feedback on the file.

BTW, should we add your example link already, or would you want to do this after your PR gets merged (I am fine both ways)?

@mdomale
Copy link

mdomale commented Jan 13, 2024

@bigludo7 Except minor comments , mentioned feature file in PR camaraproject/DeviceLocation#119 (review)
LGTM @shilpa-padgaonkar

@bigludo7
Copy link
Collaborator

Thanks a lot @shilpa-padgaonkar and @mdomale for your review.

Regarding:

BTW, should we add your example link already, or would you want to do this after your PR gets merged (I am fine both ways)?

I suggest we wait from Device location project feedback but I will inform then that current proposal is "OK" from our perpsective. Adding @jlurien in the loop.

documentation/API-Testing-Guidelines.md Outdated Show resolved Hide resolved

## Best practices and recommendations <a name="recommendations"></a>

* One feature file per API is advisable so that all scenarios can be covered corresponding to the API & the corresponding resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not impose one single .feature file per API, but one feature file per operation or API endpoint/resource. In general, it is not advisable to test several unrelated "API features" within the same .feature file. For closely related operations we may add scenarios in the same file testing crossed behaviour between several operations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shilpa-padgaonkar following today Commonalities meeting I proposed to update this sentence but I'm not able to update the file. My proposal for line 25:
Granularity of the feature file must be decided at the project level but it is recommended to:

  • group in one file all scenarios for one given API capability (that can cover several endpoints).
  • provide several files when one CAMARA API (yaml) covers several independent functions that can be provided independently

Perhaps @jlurien could provide a better wording. I think we're aligned.


## References <a name="references"></a>

* [One feature file per API](https://www.testquality.com/blog/tpost/v79acjttj1-cucumber-and-gherkin-language-best-pract)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this reference recommend having one file per API? APIs are not explicitly mentioned, but considering several files for specific parts is advised:

How should the files be separated?
Because separating the features might result in a large number of files, you must consider how to separate the features into various files.

A popular solution is to have a file with features that collect everything relevant to a specific part of the program and even organize it into directories.

In the issue #94, another reference was mentioned. There, it is advised to:

Putting multiple features in a single feature file is not recommended. It is recommended to have a separate feature file for each feature. However, it is acceptable to include multiple scenarios in a single feature file.

So we should identify what are the "features" in the context of an API, and put a feature per file. To me a feature in this context is an API operation or resource. For simple APIs with single operations, it is quite straight-forward to decide that one file is enough, e.g. location-verification API, but for more complex APIs, such as QoD, we may need several files to test unrelated features, e.g. discovery of QoD profiles, creating QoD sessions, etc

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdomale. In my understanding, the guidelines there does not advice for one feature per API but one "feature" per *.feature file. So that is what I suggest, to identify which are the "features" to test in the context of an API, and put each feature in each own *.feature file. Thus, an API may have several *.feature files for testing different aspects of the API. In simple APIs with just one endpoint that will not be the case.

Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
@shilpa-padgaonkar
Copy link
Collaborator Author

@jlurien : Thanks for your comments. I will fix the incorrect link with the new one provided by @mdomale and add it to the doc.
As you have given the example of qod api to support the case of using multiple feature files (feature in your context is an API operation or resource), I would like to ask for some additional feedback (especially from the qod codeowners and/or maintainers) to get more view points on this topic.
Tagging here the codeowners: @hdamker @RandyLevensalor @eric-murray

@bigludo7
Copy link
Collaborator

@shilpa-padgaonkar I tend to agree with @jlurien that we should flex the following sentence:

One feature file per API is advisable so that all scenarios can be covered corresponding to the API & the corresponding resource.

Indeed, as I'm involved in Device Status for example, we have there one API (one yaml) that feature 3 distincts capabilities:

  • Get on the fly if the mobile is in roaming situation
  • Get on the fly if the mobile is reacheable (connectivity)
  • Subscription to get notification when mobile change roaming/connectivity state

(the granularity of this API is perhaps challengeable but that's another topic)

I assume that it will be in this case simpler to have 3 dedicated features --> I will trigger the discussion in this project.

Let's see the feedback we have from QoD & Device Status... Perhaps we can put in like this: Granularity of the feature file should be at the project level but it is recommended to group in one file all scenario for one given API fonction. If a CAMARA API features several independent fonctions it is recommended to provide several files.

@shilpa-padgaonkar
Copy link
Collaborator Author

@bigludo7 : Thanks Ludovic for driving this in devicestatus. :)
Now awaiting feedback from devicestatus and qod

@jlurien
Copy link
Contributor

jlurien commented Jan 19, 2024

I agree with @bigludo7 and Device Status is a very good example. "Feature" is a very abstract concept, but the common advice is to not mix unrelated functionality in the same file. It is more easy to work with several small files dedicated to test an specific part of an application or API, in our case, that having only a huge file with hundreds of unrelated scenarios.
Cucumber and other frameworks accept a folder as input and are prepared to look for and execute several *.feature files in one run. So I'm in favor of relaxing the guidelines and let each subproject decide on how many "features" can be identified for some specific API.

@shilpa-padgaonkar
Copy link
Collaborator Author

@jlurien and @bigludo7 : Could you also kindly propose a text in the meanwhile which would reflect this more relaxed guideline, as opposed to "One feature file per API is advisable so that all scenarios can be covered corresponding to the API".

As we need to agree and merge this document for the upcoming release, I would request you to already prep for this text and once we get this similar feedback as suggested above from devicestatus and qod, we can add your text and proceed with approving and merging this doc.

@bigludo7
Copy link
Collaborator

@jlurien and @bigludo7 : Could you also kindly propose a text in the meanwhile which would reflect this more relaxed guideline, as opposed to "One feature file per API is advisable so that all scenarios can be covered corresponding to the API".

As we need to agree and merge this document for the upcoming release, I would request you to already prep for this text and once we get this similar feedback as suggested above from devicestatus and qod, we can add your text and proceed with approving and merging this doc.

I've just provided a sentence as a review comment ;)

@shilpa-padgaonkar
Copy link
Collaborator Author

@bigludo7 : Thanks. Your suggestions are now added to the doc :)

Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

LGTM

@shilpa-padgaonkar
Copy link
Collaborator Author

shilpa-padgaonkar commented Jan 22, 2024

Requesting @jlurien to kindly review and approve if ok

Copy link
Contributor

@jlurien jlurien left a comment

Choose a reason for hiding this comment

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

LGTM with some minor suggestion

documentation/API-Testing-Guidelines.md Outdated Show resolved Hide resolved
Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
@shilpa-padgaonkar
Copy link
Collaborator Author

Thanks all for the contributions, review and approval. @rartych: Could you kindly check that there are no more open points ignored as a part of the review and if not, proceed to merge the PR?

@rartych
Copy link
Collaborator

rartych commented Jan 24, 2024

@RubenBG7 @jordonezlucena - as @jlurien approved the PR, could you approve it as codewoners?

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.

Test definitions - implementations guidelines and artifacts
8 participants