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

GPII-3333: Improve production tests to cover more functionality #718

Merged
merged 78 commits into from
Apr 23, 2019

Conversation

klown
Copy link
Member

@klown klown commented Dec 7, 2018

JIRA: https://issues.gpii.net/browse/GPII-3333

Adds tests for cloud based flowmanager end points:

  • /login and /logout
  • /health
  • /ready
  • /access_token
  • /%gpiiKey/settings/%device
  • /%gpiiKey/settings

Note: Includes changes for GPII-3551 (PR #713)

@gpii-bot
Copy link

gpii-bot commented Dec 7, 2018

CI job passed: https://ci.gpii.net/job/universal-tests/1386/

- used user ID veriable and termMap instead of hard-coding ID,
- used directModel argument to send() for passing access token.
@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/1391/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/1396/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/1406/

config: {
configName: "gpii.tests.productionConfigTests.config",
configPath: "%gpii-universal/tests/configs"
},
components: {
healthRequest: {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are test definitions that work with development configs, which start both the local flow manager and the cloud locally, for all end points provided by the cloud. It would be beneficial for reducing the work load and future reusability if these test definitions can be reused for the production configs, where the cloud is served by docker containers.

For example, the existing tests for /settings GET is at tests/platform/cloud/SettingsGetTests.js, and for /settings PUT is at tests/platform/cloud/SettingsPutTests.js. The successful and failed cases for /access_tokens can also be found there.

Copy link
Member Author

@klown klown Jan 9, 2019

Choose a reason for hiding this comment

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

@cindyli These are the changes thus far for using the test definitions in SettingsGetTests.js for both development and production configurations. It's still rough, but I'm pushing it to see if it works in CI.

…ction testing

Refactoring and defining common test defintions based on existing development
settings tests for use with production testing of the cloud based flow manager.

Note:  too early for use; committing to save the work thus far.
Re-factored and tweaked to have the capability of runnning the
"setttings set" tests in either development or production mode.
Updated the developement tests for "put settings" to work with the
changes introduced for running "get settings" tests in both
development and production configuration.
Renamed to keep the test definitions separate for the "set settings"
tests vs. the "put settings" tests.
Previous refactoring of test definitions and code for use in a
production configuration requires local declaration of a development
configuration for developement tests.
@gpii-bot
Copy link

gpii-bot commented Jan 9, 2019

CI job passed: https://ci.gpii.net/job/universal-tests/1429/

@@ -12,9 +12,9 @@

"use strict";

var fluid = require("infusion"),
var fluid = require("infusion"),
Copy link
Member

Choose a reason for hiding this comment

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

Clownspacing has arisen

Copy link
Member Author

Choose a reason for hiding this comment

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

I protest my innocence, as I didn't directly touch this file. The extra space is due to merging with @the-t-in-rtf 's GPII-3619 branch before it went into master. See:

gpii = fluid.registerNamespace("gpii");
@cindyli noted a few similar issues above.

Perhaps the definition of "Clownspacing" is the act of copying other author's mistakes before they make a final cleanup for merging into master. I can live with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please still remove these extra spaces at line 15 and 17. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Used npm to run the production configuration tests with separate
invocations of npm to add and remove the test gpii keys and prefsssafes.
@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/1658/

@gpii-bot
Copy link

CI job failed: https://ci.gpii.net/job/universal-tests/1659/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/1660/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/1669/

@klown
Copy link
Member Author

klown commented Mar 27, 2019

@cindyli ready for review -- thanks.

gpii.tests.cloud.oauth2.settingsPut.updateSnapset.testLifecycleResponse = function (data, request) {
gpii.tests.cloud.oauth2.settingsPut.updateSnapset.testStatusCode(data, request);

var lifeCycle = JSON.parse(data);
Copy link
Member

Choose a reason for hiding this comment

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

Could you normalise capitalisation of "lifeCycle" to "lifecycle" throughout, consistent with usage in the core. Note that this function itself already uses "Lifecycle" in its name. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/1677/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/1678/

});

// Tests for deleting test 'user' preferences from the database
gpii.tests.productionConfigTesting.deleteTestRecordsFromDatabaseTests = [{
Copy link
Contributor

@cindyli cindyli Mar 29, 2019

Choose a reason for hiding this comment

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

Sorry I should have thought about this much earlier rather than discovering it now. Long story in short, only deleting these records is not enough.

If you look into the CouchDB after running "npm run test:vagarantProduction", there are still some new records in there:
Screen Shot 2019-03-29 at 2 36 05 PM

Open one of them, it looks like below and all these new records are in this structure:
Screen Shot 2019-03-29 at 2 36 46 PM

These are access tokens assigned to OAuth clients. Refer to the documentation about OAuth authentication GPII supports. Every GET or PUT requests to /settings endpoint requires a valid access token first. This is when they are created. They need to be removed from the database too when the production test completes.

Copy link
Member Author

@klown klown Mar 29, 2019

Choose a reason for hiding this comment

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

Followup, based on a teleconference with @cindyli

  • Add a new view to _design/views that will find these access tokens based on:
    -- gpiiKey
    -- clientCredentials
    -- type (always "gpiiAppInstallationAuthorization")

(Note: The gpiiKey and clientCredentials are the same as those used in the test scenario that created the access_token in the first place.)

Also, this new view will be of use for GPII-3646 where we free up the database of all the expired access tokens created from day-to-day use of the system.

@gpii-bot
Copy link

gpii-bot commented Apr 2, 2019

CI job passed: https://ci.gpii.net/job/universal-tests/1684/

@klown
Copy link
Member Author

klown commented Apr 2, 2019

The technique to remove the extra access tokens is okay, but could be improved. It uses the GPII Key IDs used in the tests to find a subset of access tokens in the database and remove that group. One ID, however, is used elsewhere, namely "carla"; so this will remove all of carla's access tokens, even ones created outside of these tests.

An improvement is to use the timestamps within the access tokens, relying on the fact that the "test" access tokens were all created within the last hour (practically speaking, within the last five minutes, or less).

Ideally, the code should remove all-and-only the access tokens that were created by the tests. But, because the database cleanup code is run in a separate node process, there is no way to store token references in memory and share them with the cleanup code. The references could be written to disk and read in by the deletion code, but that feels clunky.

@klown
Copy link
Member Author

klown commented Apr 8, 2019

A followup to my comments above. I wrote:

Ideally, the code should remove all-and-only the access tokens that were created by the tests.

Note that the set-up code in vagrantCloudBasedContainers.sh also asks for an access token for "carla" as a way to make sure the flowmanager container is ready. That check also creates a "spurious" access token.

Why not use the flowmanager's /ready endpoint instead?

@gpii-bot
Copy link

gpii-bot commented Apr 9, 2019

CI job passed: https://ci.gpii.net/job/universal-tests/1698/

Improved technique to remove only the access tokens create by the
tests.
@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/1705/

@klown
Copy link
Member Author

klown commented Apr 16, 2019

The latest technique is an improvement over the previous one.. It stores the access tokens that the tests create when making flowmanager /access requests, and deletes them when the tests using them are done.

A few access tokens are still missed, however:

  • There are a pair of login/logout tests that create access tokens as a side effect. It's not clear how to catch them since the request is /user/:gpiiKey/login and not a direct /access request,
  • As noted previously, the shell script vagrantCloudBasedContainers.sh tests the readiness of the database by issuing a flowmanager /access request, but that's well in advance of the test code, and could be replaced with a flowmanager /ready request (which does not create access tokens as a side effect).

- modified vagrantCloudBasedContainers.sh to use the flowmanager's
"/ready" endpoint to check its readiness rather than using its
"/access_token" endpoint -- avoids creation of extra access token,

- modified the deletion of extra gpii keys and pref safes to also
remove the access tokens for the login tests for "testUser1" and
"nonexistent_gpii_key".
@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/1723/

@klown
Copy link
Member Author

klown commented Apr 22, 2019

These modifications take care of the three remaining extra access tokens created by the tests.

Ready for reivew, @cindyli

@cindyli cindyli merged commit 074a835 into GPII:master Apr 23, 2019
@cindyli
Copy link
Contributor

cindyli commented Apr 23, 2019

Merged at 78a7e7a

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.

None yet

5 participants