Replace requestConfig with routeMatcherMap in helper functions for integration tests by pedrottimark · Pull Request #3686 · stackrox/stackrox

pedrottimark · 2022-11-03T21:28:29Z

Description

Follow up removal of need for optional requestConfig properties

waitOptions in ROX-12956: Increase requestTimeout and disable colors for integration tests #3307
opnameAliasesMap in Distinguish queries with same opname in compliance integration tests #3627

Replace occurrences of requestConfig with its only remaining routeMatchMap property.

Here is a first draft of documentation. Review also requested on where to make it more correct, clear, and complete (for the primary audience of test readers and writers, including from the viewpoint of part-time contributers from backend teams).

TL;DR

Why should tests wait for requests?
- Negative: Remove timing as a reason for tests to fail.
- Positive: Declare the most relevant data dependencies.
Replace imperative pattern with declarative pattern.
- Replace cy.intercept and cy.wait in tests, or even in container helpers, with routeMatcherMap and staticResponseMap which organize Cypress data types in parallel objects.
- Be able to wait on all relevant requests for container, but mock only relevant responses for test (for example, widget on main dashboard).
Encapsulate details in layers:
- Factor out how to wait and mock as generic helpers. This is prerequisite knowledge for all tests.
- Encapsulate what to wait for: generic requests to render any page. Ditto.
- Encapsulate what to wait for container-specific requests with page and endpoint addresses. Also select selectors to verify pages, pardon pun. Also be able to encapsulate and compose interaction at source container to reach target container. For example, from Risk deployment to Network Graph. Limit knowledge for container tests to container helpers and container constants. Minimize import from constants/apiEndpoints.js file.

Declarative specification of relevant requests

Cypress RouteMatcher: https://docs.cypress.io/api/commands/intercept#Matching-with-RouteMatcher

For example

REST request to save changes on System Configuration page:

{
    method: 'PUT',
    url: configEndpoint,
}

GraphQL request helper function:

export function getRouteMatcherForGraphQL(opname) {
    return {
        method: 'POST',
        url: `/api/graphql?opname=${opname}`,
    };
}

Test helpers for workflow entities wait on the only predictable request via 3 maps of plural opname, singular opname, and entity type for primary-secondary requests (although a particular page or side panel might make other requests).

For example, plural entities in Vulnerability Management:

const opnameForEntities = {
    clusters: 'getClusters',
    components: 'getComponents',
    'image-components': 'getImageComponents',
    'node-components': 'getNodeComponents',
    cves: 'getCves',
    'image-cves': 'getImageCves',
    'node-cves': 'getNodeCves',
    'cluster-cves': 'getClusterCves',
    deployments: 'getDeployments',
    images: 'getImages',
    namespaces: 'getNamespaces',
    nodes: 'getNodes',
    policies: 'getPolicies',
};

Declarative specification of mock responses

Cypress StaticResponse: https://docs.cypress.io/api/commands/intercept#StaticResponse-objects

response body: { body: { collections: […] } }
fixture file: { fixture: 'collections/collectionsWhatever.json' }

Maps for requests and responses

Now routeMatcherMap and staticResponseMap have parallel structure:

key consists of either
- whateverAlias for REST endpoint: delete /v1/ and usually omit search query (and prepend with method if other than GET)
- whateverOpname for GraphQL endpoints: delete /api/graphql?opname=
value consists of RouteMatcher or StaticResponse object

The keys in staticResponseMap object are often a subset of keys in routeMatcherMap object.

Collections example

Collections need mock responses until endpoints are implemented.

Unless there are default collections, many tests will still need mock responses.

Why not 'count' for the second alias? Prefer more specific:

partly to prevent alias collisions.
mostly to match up alias with endpoint at the left in Cypress dashboard.

Also, prefer to encapsulate addresses in container helpers file:

Prevent leakage of details outside of the container.
Reduce the number of files to read or write tests.

const basePath = '/main/collections';

export const collectionsAlias = 'collections';
export const collectionsCountAlias = 'collections/count';

const routeMatcherMapForCollections = {
    [collectionsAlias]: {
        method: 'GET',
        url: '/v1/collections?query=*',
    },
    [collectionsCountAlias]: {
        method: 'GET',
        url: '/v1/collections/count?query=*',
    },
};

// visit

export function visitCollections(staticResponseMap) {
    visit(basePath, routeMatcherMapForCollections, staticResponseMap);

    // generic assertions
}

const collections = [];
const count = collections.length;

const staticResponseMap = {
    [collectionsAlias]: {
        body: { collections },
    },
    [collectionsCountAlias]: {
        body: { count },
    },
};

    it('should have table column headings', () => {
        visitCollections(staticResponseMap);

        cy.get('th:contains("Collection")');
        cy.get('th:contains("Description")');
        cy.get('th:contains("In use")');
    });

Here is an important, although invisible part of the pattern:

Generic helper functions have optional routeMatcherMap and staticResponseMap arguments. Caller is usually in another helper file (see item 2) except some tests might also call interactAndWaitForResponses function.
- interactAndWaitForResponses in request.js
- visit in visit.js
- visitFromLeftNav or visitFromLeftNavExpandable in nav.js
Container-specific helper functions have encapsulated route matcher map and optional staticResponseMap argument. Helpers file exports whateverAlias or whateverOpname constants as keys.

Checklist

Investigated and inspected CI test results
Edited integration tests

Testing Performed

ghost · 2022-11-03T22:01:40Z

Images are ready for the commit at 8765284.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.72.x-628-g8765284718.

dvail

Thanks for this enhancement Mark. Declarative request/response configs is cleaner and less error-prone than the intercept/wait sandwich that was required throughout the tests otherwise.

As for documentation review in the next draft, I think the biggest two things that would be valuable to have would be:

An easily findable section in the testing docs that explains that these helpers exist and why we use them over the lower level Cypress equivalents.
A section that adds "When should tests wait for requests?" to "Why should tests wait for requests?"

For the second one I mean guidelines for part-time contributors on when they might want to use these request matchers versus doing something like using Cypress's built in retry mechanism to drive the tests.

In the example you provided for Collections:

    it('should have table column headings', () => {
        visitCollections(staticResponseMap);

        cy.get('th:contains("Collection")');
        cy.get('th:contains("Description")');
        cy.get('th:contains("In use")');
    });

this is a case where my instincts would be to just visit the page and run the cy.get commands directly without intercepting or waiting for requests since the existence of those th elements ensures that the required requests have been completed. Something like:

    it('should have table column headings', () => {
        cy.visit(collectionsUrl);

        cy.get('th:contains("Collection")');
        cy.get('th:contains("Description")');
        cy.get('th:contains("In use")');
    });

I suppose this might be something that is tough to have guidelines on, since the only people with in depth knowledge of the data dependencies of each page are likely to be members of the UI team. Or is the expectation that since we will already have these visit helpers defined, that we should always use them as the first function call in test code?

dvail · 2022-11-14T18:24:40Z

ui/apps/platform/cypress/helpers/request.js

@@ -1,35 +1,59 @@
+/*
+ * For pages which have GraphQL and REST requests.


This comment is a little confusing, given that this function is called from getRouteMatcherMapForGraphQL below, which has a comment that states that it is for pages that only have GraphQL requests. Would it be worth it to simplify the number of exported utils and inline this function with the latter?

It looks like the use case this function serves could be replaced with an object spread:

{ [summaryCountsOpname]: getRouteMatcherForGraphQL(summaryCountsOpname), [getAllNamespacesByClusterOpname]: getRouteMatcherForGraphQL(getAllNamespacesByClusterOpname), }

becomes

{ ...getRouteMatcherMapForGraphQL([summaryCountsOpname, getAllNamespacesByClusterOpname]), }

Or are there cases where we would want the opname in the routeMatcherMap to be different from the opname appended to the URL?

Yes, good eyes and super suggestion. I will reflect and rewrite to suggest spread.

Which reminds me that for main dashboard, which is the most complicated request map, I will replace comments with spreads, something like:

const routeMatcherMapForMainDashboard = { ...routeMatcherMapForSummaryCounts, ...routeMatcherMapForSearchFilter, ...routeMatcherMapForViolationsByPolicySeverity, ...routeMatcherMapForImagesAtMostRisk, ...routeMatcherMapForDeploymentsAtMostRisk, ...routeMatcherMapForAgingImages, ...routeMatcherMapForViolationsByPolicySeverityOrCategory, // countsByCategory for both? ...routeMatcherMapForComplianceLevelsByStandard, }

By happy circumstance, it looks like none of the sub-maps have both GraphQL or REST.

I need to search again to double-check that most requests are disjoint, except maybe the one for which I wrote a pseudo-comment.

dvail · 2022-11-14T18:38:12Z

ui/apps/platform/cypress/helpers/visit.js

-         * because they are in parallel with (and possibly even delayed by) page-specific requests.
-         */
-    },
+const routeMatcherMapGeneric = {


Possible naming nit carried over from the last iteration: the Generic suffix to me almost implies that these are configurable, or "generic" in some way, when they are actually "specific" routes to match what happens to occur on every page.

What do you think about routeMatcherMapGlobal, routeMatcherMapUniversal, routeMatcherMapDefaults or something similar to convey that this is an implementation of the net requests that are fired regardless of what page you navigate to?

“You never get a second chance to make a first impression.” Yes, we will improve the suffix before I merge.

I renamed as routeMatcherMapForAuthenticatedRoutes

dvail · 2022-11-14T18:42:05Z

ui/apps/platform/cypress/helpers/main.js

-export const getImagesAlias = 'getImages';
-export const deploymentswithprocessinfoAlias = 'deploymentswithprocessinfo';
-export const agingImagesQueryAlias = 'agingImagesQuery';
+export const summaryCountsOpname = 'summary_counts';


snake_case, everythinglowercase, and camelCase opnames all gathered together 😢

Yeah, for sure. I just found some more irregularities while writing a draft of integrations helpers.

It we write tests sooner in declarative style, we might be able to smooth out more rough edges.

dvail · 2022-11-14T18:47:24Z

ui/apps/platform/cypress/integration/logout.test.js

+const routeMatcherMapForLogout = {
+    [logoutAlias]: {
+        method: 'POST',
+        url: '/sso/session/logout',


+1

After reading through these files I definitely think colocating the URL with the routeMatcher is the way to go. One less file to have to jump through to find these URLs will make debugging easier.

dvail · 2022-11-14T19:03:32Z

ui/apps/platform/cypress/helpers/request.js

+    return routeMatcherMap;
+}
+
 /*


Request for JSDoc comments if we want some extra type goodness:

Currently in VSCode when viewing this section, although the parameters are well typed in the comments we do not get any help from the editor in consuming code. See request.js on the left and hovering over interceptRequests() in visit.js on the right below:

Adding the second star to the start of the comment block will give us better highlighting, documentation on hover, as well as type inference in consuming files:

We can also get some warnings for function misuse, although these would not be as reliable as the app code that goes through the full TS toolchain:

I was going to suggest adding = to the types as well, to indicate that they are optional

// e.g. // @param {Record<string, { method: string, url: string }>} [routeMatcherMap] // becomes // @param {Record<string, { method: string, url: string }>=} [routeMatcherMap]

but it looks like TypeScript can already infer this now based on the surrounding code!

Aha, so that is the meaning of the second star! Thank you for critique and pictures.

Ouch, I missed that nuance when we rewrote service functions in TypeScript.

With /** Visual Studio Code interprets brackets [routeMatcherMap] as optional.

I had searched that specific detail online but missed the obvious notation.

…tegration tests

… for networkFlows

7ffc6be ROX-13368: Skip failing nongroovy tests on PG (#3721) bbdd7a0 Bump github.com/gofrs/uuid from 4.3.0+incompatible to 4.3.1+incompatible (#3642) 1f253f2 Bump github.com/google/certificate-transparency-go from 1.1.3 to 1.1.4 (#3543) d434c8d [ROX-13030] : Add delete collection API endpoint and service implementation (#3648) f062c21 Dashrews/ROX-13253 wait for central-db to come back after bounce and allow FATAL connection lost error (#3537) edc1174 CI: Fill the gaps for https://testgrid.k8s.io/ (#3715) 86d7c54 ROX-13231: use passed context when non-postgres (#3540) 9093195 Add less specific type for BE collection response string (#3728) 5abb652 Only enable ROX_OBJECT_COLLECTIONS feature flag during gke-postgres-ui-e2e job (#3727) 4f64cd1 Add centralDBOnly mode in render (#3707) d67bbe5 Dashrews/ROX-13082 UUID searcher and common updates to set allow use of postgres UUID PR 1 of 4 (#3679) 6f829d5 ROX-13259: graphInit called during init time (#3705) f3bc50d ROX-13380: Conditional rendering edges for deployments and namespaces (#3641) 3764476 ROX-12319: implement smoke test step with groovy test filter (#3220) f202fd4 ROX-11826: Disable kernel support package uploads for managed central (#3661) 61f03dc ROX-11101: Remove deprecated resources from central (#3115) e6aa6d7 ROX-11101: Restore Role permission in UI (#3428) 3203e04 ROX-11101: Remove deprecated resources (#3036) a35f41e Bump golang.org/x/sys from 0.1.0 to 0.2.0 (#3733) 4120524 Bump snakeyaml from 1.29 to 1.33 in /qa-tests-backend (#3732) e1785c0 Bump github.com/coreos/go-systemd/v22 from 22.4.0 to 22.5.0 (#3724) 870df4a Bump google.golang.org/api from 0.101.0 to 0.102.0 (#3723) 721454c Generalize User-Agent setup (#3672) 6a11bf0 Bumps collector version to 3.11.x-145-gc345f72f5e (#3736) ec5d343 [ROX-12923] Walk retries - remainder work (#3729) 40f3d43 ROX-13440: Replace ambiguous central with sensor in networkGraph integration test (#3730) 281ed22 Bump groovy-xml from 2.5.18 to 2.5.19 in /qa-tests-backend (#3741) 49d1651 Bump github.com/prometheus/client_golang from 1.13.1 to 1.14.0 (#3742) b5544aa Bump cloud.google.com/go/storage from 1.27.0 to 1.28.0 (#3743) 9c61e53 ensure CVSS is present for istio vulns (#3706) ae29d52 ROX-13452: don't always clobber scoped ctx when non-postgres (#3748) 517bf05 ROX-13261: DryRunUpdate on collection datastore (#3687) baf7654 ROX-13378: Group new resources with deprecated in UI (#3690) 569922f ROX-13421: Enable roxctl netpol generate and add tech-preview messages (#3740) 2465fc5 Dashrews/ROX-13082 UUID generator templates PR 2 of 4 (#3681) c093c68 Bump slack-api-client from 1.20.2 to 1.27.0 in /qa-tests-backend (#3752) 2c860bb Bump ubi8-micro from 8.6 to 8.7 in /operator (#3751) 80eb04c Make deploy.sh and deploy-local.sh pass shellcheck (#3582) 2182b43 Dashrews/ROX-13082 UUID test updates PR 3 of 4 (#3694) 6dc6ca5 [ROX-13403] : Fix node -> topVuln sub resolver bug when node cves is empty (#3689) 1b21361 Move integration tests for page title from general to specific containers (#3675) e1a9f31 Bump google.golang.org/api from 0.102.0 to 0.103.0 (#3773) a05ea31 Bump golang.org/x/crypto from 0.1.0 to 0.2.0 (#3772) 65ddf4f ROX-12824: Add roxctl commands to generate Central DB bundle (#3602) c3f1e2f Remove obsolete authProviders request for Integrations page (#3759) 7ccd54d Dashrews/ROX-13082 UUID protos generated PR 4 of 4 (#3698) 9ab5c8f cleanup image digest utilities (#3764) 187ed44 ROX-11931: Convert junit failure artifacts to Slack attachments (#3438) b5d8790 ROX-13432: leaning up unused code copied/pasted from topology demo (#3750) ab05bfc Refactor collection form page for better composition (#3744) c5562f7 Remove babel devDependencies in ui-components (#3761) 2b90b3a Extract collection form from drawer wrapper layout (#3745) a779fc9 [ROX-12625 + ROX-13032] : Add GetCollectionCount and UpdateCollection endpoints and services (#3749) e77f0da Upgrade cypress 11.0.0 devDependencies in ui (#3760) a3fba94 ROX-13068: Use real data for deployment details (#3688) 4c7d90e ROX-12617: Collection to search query converter (#3683) 3e98aec ROX-13067: fill out port configurations section of deployment details (#3714) a48de36 ROX-12835: Add support for NodeScanV2 to Sensor (#3533) 30c5dc7 ROX-13466: Fix deletion of groups with empty properties (#3756) 5cb2470 Add autocomplete for name selector dropdowns (#3676) b9a75ad ROX-13464 adding flows dropdown in NG (#3763) 3217a67 [ROX-13500] Perform type check for V1 CronJob (#3787) af3790d Remove bulk delete from collections table (#3776) dda123b Add more info in migration log (#3788) 179f0c9 ROX-13502: Remove the circular dependency between cluster datastore init and cscc notifier init (#3790) 029d584 Update SCANNER_VERSION (#3774) cbca57c Bump github.com/ckaznocha/protoc-gen-lint from 0.2.4 to 0.3.0 (#3783) 3613b56 Bump golang.org/x/tools from 0.2.0 to 0.3.0 (#3782) 5fc0a6a Bump github.com/google/go-containerregistry from 0.12.0 to 0.12.1 (#3781) 1d1c687 Bump controller-gen version to 0.10.0 (#3754) c3a5290 Untie documentation link from the product version (#3799) ed822aa use correct package for migration (#3784) 397a0b4 Validate that label keys are valid k8s labels and ensure correct key splitting (#3777) edd1050 Rename variable ScannerGRPCEndpoint to ScannerSlimGRPCEndpoint (#3657) 6662c9f ROX-13378: Access Control page permissions (#3720) b0e73c5 fix Operator reconciliation for external Central DB (#3796) b83bc1f ROX-13505: Fix error log scanning the postgres stat collection (#3795) ca660cb Prevent the collection being edited from displaying in its own embedded list (#3778) 3f7b3fc [ROX-13441][POSTGRES] Propagate context correctly in retries (#3793) e0cbc6f ROX-12839: Update changelog to announce removal of in-product docs (#3805) 696e8bc [ROX-12358] Follow up on vulnerability request proto change (#2851) c4b46d8 Change getCollectionCount endpoint and updateCollection request type 5f2efbc remove make proto-fmt (#3804) 0c75540 Remove os.Std* from roxctl/central (#3758) 25a90de Add ability to view embedded collections in a pop up modal (#3747) 5c1bf81 ROX-13240: fix scanner-slim updates when WebSockets are used (#3704) 1d98577 Add more context to jira notifier logging (#3812) da2fd28 ROX-13031: DryRun Collection API (#3766) 1c418d5 Test data migration code in postgres tests (#3803) ed95b37 Update UI Collection requests for BE compatibiltiy (#3762) 09cc188 ROX-11931: Fix junit-parse install in CI (#3811) d2b01e3 ROX-12814: Disable PolicyFieldsTest on openshift. (#3797) d10ce27 ROX-13345: disable 'missing required registry' aspect on openshift (#3798) 3d22396 Update collector to 3.12 (#3809) 1eb33fb ROX-13347: Modify scope queries to included quoted cluster and nameace names, to allow exact matches instead of erroneous and unintended prefix matches. (#3767) 3811a69 ROX-12621: list collection selectors api (#3806) f6d3f9d Add migration for groups with invalid values (#3789) cc21125 Bugfixes for collection autocomplete (#3816) 7623dec ROX-9350 Use fine-grained host paths for compliance mounts (#2479) b4bf5c2 Fix collector volumeMounts (#3826) 0e9be05 ROX-12953: figure out last 4 versions of sensor automatically (#3611) 459c7ae ROX-12814: Add proper todo for reenabling the test (#3817) 9ee40ff ROX-13523: add isEnabled enum to central db spec (#3815) 535bc72 Replace requestConfig with routeMatcherMap in helper functions for integration tests (#3686) 2b75b61 `gosec` G104: Add `ShouldErr(err)` that returns `err` (#3830) fb1b82f WIP: Introduce nodescan call 35f8a8f WIP: Prepare converter 716144b Moved and renamed fake nodescan tests 4748de7 Introduce real node scanner with conversion functions 5e6d9a8 wip: real scanner 0169868 wip: log results 1438d2c wip: Debug Analyze call b09894c wip: Debug Analyze call 3ceed72 wip: Update and improve debug logs d1669fd Remove copied lib, bump scanner version, add debug 14a3f73 Merge branch 'master' into mm/ROX-12967-real-nodescan fbd0450 Fix style issues 17ccb31 Debug: let both scans finish to see what they return

github-actions bot added the area/ui label Nov 3, 2022

pedrottimark requested a review from dvail November 14, 2022 17:06

dvail approved these changes Nov 14, 2022

View reviewed changes

Mark Pedrotti added 10 commits November 15, 2022 16:27

Replace requestConfig with routeMatcherMap in helper functions for in…

a8b287d

…tegration tests

Rename routeMatcherMapForComplianceDashboard

915d280

Rename routeMatcherMapForConfigurationManagementDashboard

f8db6c7

Fix lint error

958a6e3

Rename routeMatcherMapForVulnerabilityManagementDashboard

2fc8512

Replace implicit requestConfig objects with routeMatcherMap

be1a11e

Replace implicit routeConfig with routeMatcherMap in helper functions…

b306eeb

… for networkFlows

Replace getRouteMatcherForGraphQL with object spread

eb674d2

Replace Generic with AuthenticatedRoutes in visit helpers file

46f54d2

JSDoc comments in main nav request visit helpers

8765284

pedrottimark force-pushed the cypress-replace-requestConfig branch from a4bb6cf to 8765284 Compare November 15, 2022 21:35

pedrottimark merged commit 535bc72 into master Nov 16, 2022

pedrottimark deleted the cypress-replace-requestConfig branch November 16, 2022 14:14

		@@ -1,35 +1,59 @@
		/*
		* For pages which have GraphQL and REST requests.

Conversation

pedrottimark commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Declarative specification of relevant requests

Declarative specification of mock responses

Maps for requests and responses

Collections example

Checklist

Testing Performed

Uh oh!

ghost commented Nov 3, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvail 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pedrottimark commented Nov 3, 2022 •

edited

Loading

ghost commented Nov 3, 2022 •

edited by ghost

Loading