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

[API] New branch for adding the testsuite for the API + addressed comments #6671

Merged
merged 97 commits into from Aug 27, 2020

Conversation

spell00
Copy link
Contributor

@spell00 spell00 commented Jun 4, 2020

Creation of a test suite for the API.

Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

Some change requested.

Overall, I see that you are trying to programatically test each endpoints from an array on strings. Whilst this could be a good way to save lines of code, it creates two really convoluted functions (_allNamesGet and _allNamesPost)

Since we are using PHPUnit to run our test, it is good practice to have a test for each functionality/feature. I would prefer to have a test... function for each endpoint that would allow more specific handling and more control over the cyclomatic complexity of our code. (@johnsaigle)

ex:

public function testCandidatesPOST(): void
{
    $payload = ['DoB': "2010-01-01", ....];
    $response = $this->_client->request('POST', $this->_base_uri . '/candidates', $payload);
    $this->assertEquals($response->getStatus(), '201', 'Candidate not created');
    $body = json_decode($response->getBody()->getContent(), true);
    $this->assertArrayHasKey($body, 'CandID');
}

Otherwise, if you want to keep the array of endpoints, PHPUnit also provides a Data provider functionality that allows to create an array of parameters and expected results to pass as parameters of tests functions.
see: https://phpunit.readthedocs.io/en/9.0/writing-tests-for-phpunit.html?highlight=generator#data-providers

modules/api/test/ApiTest.php Outdated Show resolved Hide resolved
modules/api/test/ApiTest.php Outdated Show resolved Hide resolved
modules/api/test/ApiTest.php Outdated Show resolved Hide resolved
modules/api/test/ApiTest.php Outdated Show resolved Hide resolved
@spell00 spell00 added Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix API PR or issue introducing/requiring modifications to the API code Testing PR contains test plan or automated test code (or config files for Travis) labels Jun 5, 2020
@spell00
Copy link
Contributor Author

spell00 commented Jun 5, 2020

Thank you for your feedback @xlecours , I'll address your comments soon

@ridz1208 ridz1208 modified the milestone: 23.0.0 Jun 8, 2020
@christinerogers
Copy link
Contributor

@spell00 will this be ready for the 23 release or should it be rebased to master ?

@spell00
Copy link
Contributor Author

spell00 commented Jun 11, 2020

@christinerogers It will not be ready for the 23 release

@christinerogers
Copy link
Contributor

Ok. please rebase to master

@christinerogers christinerogers added the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Jun 12, 2020
@ridz1208 ridz1208 modified the milestone: 23.0.1 Jun 12, 2020
@spell00 spell00 changed the base branch from 23.0-release to master June 15, 2020 18:10
@spell00
Copy link
Contributor Author

spell00 commented Jun 15, 2020

Rebase to master is done

@christinerogers christinerogers removed the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Jun 16, 2020
@xlecours xlecours self-assigned this Jul 2, 2020
Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

Here are some comments and suggestions.
It is much more explicit than the previous version 👍

I would recommend changing LorisApiTest.php for LorisApiAuthenticatedTest.php and to use the setUp mechanics of phpUnit so that guzzleLogin do not need to be explicitly called at the begining of each test.

modules/api/test/LorisApiTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiCandidatesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiCandidatesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiCandidatesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiCandidatesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiDicomsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiDicomsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiDicomsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiDicomsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiDicomsTest.php Outdated Show resolved Hide resolved
@spell00 spell00 force-pushed the 2020-06-04-AddCleanApiTestSuite branch from af361d5 to e7a325e Compare July 7, 2020 00:40
Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

More changes.
If it is too much to make all the changes, remove the response content shape assertion and we will add them in a subsequent PR.

modules/api/test/LorisApiAuthenticationTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiAuthenticationTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiAuthenticationTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiAuthenticationTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiCandidatesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiCandidatesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiCandidatesTest.php Outdated Show resolved Hide resolved
@spell00 spell00 force-pushed the 2020-06-04-AddCleanApiTestSuite branch 2 times, most recently from 0cf898a to 4ba825c Compare July 10, 2020 05:57
@spell00
Copy link
Contributor Author

spell00 commented Jul 14, 2020

Many incomplete tests are Incomplete because their is no /data-raisinbread directory in the docker container.

@spell00 spell00 requested a review from xlecours July 14, 2020 19:21
Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

So change requests. Good work so far. Let's keep going!

modules/api/test/LorisApiCandidatesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiDicomsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiDicomsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiDicomsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiDicomsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiDicomsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiImagesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiImagesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiImagesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiImagesTest.php Outdated Show resolved Hide resolved
Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

And some more :)

modules/api/test/LorisApiInstrumentsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiInstrumentsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiInstrumentsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiInstrumentsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiInstrumentsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiRecordingsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiRecordingsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiVisitsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiVisitsTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiVisitsTest.php Outdated Show resolved Hide resolved
@spell00 spell00 force-pushed the 2020-06-04-AddCleanApiTestSuite branch from cde457e to f1193b6 Compare July 21, 2020 02:50
modules/api/test/LorisApiCandidatesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiImagesTest.php Outdated Show resolved Hide resolved
modules/api/test/LorisApiRecordingsTest.php Outdated Show resolved Hide resolved
@spell00
Copy link
Contributor Author

spell00 commented Aug 24, 2020

@xlecours as we discussed, the JWT key is now modified in raisinbread/test/api/LorisApiAuthenticatedTest.php, as it is only required for the API test suite, leaving test/integrationtests/LorisIntegrationTest.class.inc unchanged.

raisinbread/test/api/LorisApiAuthenticatedTest.php Outdated Show resolved Hide resolved
raisinbread/test/api/LorisApiAuthenticatedTest.php Outdated Show resolved Hide resolved
raisinbread/test/api/LorisApiAuthenticatedTest.php Outdated Show resolved Hide resolved
raisinbread/test/api/LorisApiAuthenticatedTest.php Outdated Show resolved Hide resolved
@spell00 spell00 requested a review from xlecours August 24, 2020 19:05
raisinbread/test/api/LorisApiAuthenticatedTest.php Outdated Show resolved Hide resolved
raisinbread/test/api/LorisApiAuthenticatedTest.php Outdated Show resolved Hide resolved
@xlecours
Copy link
Contributor

@driusan This should be good to go.

@spell00 spell00 added the Blocking PR should be prioritized because it is blocking the progress of another task label Aug 26, 2020
@driusan driusan dismissed their stale review August 27, 2020 13:02

oops.. somehow I accidentally dismissed @xlecour's review, not mine.

@driusan driusan merged commit 7681b42 into aces:main Aug 27, 2020
@xlecours
Copy link
Contributor

@spell00 crongrats !

laemtl pushed a commit to laemtl/Loris that referenced this pull request Sep 3, 2020
Add test suite which uses raisinbread and guzzle to perform integration tests for the API.
zaliqarosli pushed a commit to zaliqarosli/Loris that referenced this pull request Sep 3, 2020
Add test suite which uses raisinbread and guzzle to perform integration tests for the API.
@ridz1208 ridz1208 added this to the 24.0.0 milestone Nov 27, 2020
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
Add test suite which uses raisinbread and guzzle to perform integration tests for the API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API PR or issue introducing/requiring modifications to the API code Blocking PR should be prioritized because it is blocking the progress of another task Passed Manual Tests PR has undergone proper testing by at least one peer Testing PR contains test plan or automated test code (or config files for Travis)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants