[automate-2697] stabilize project mgmt tests #3584

bcmdarroch · 2020-05-04T19:33:22Z

🔩 Description: What code changed, and why?

Stabilized existing tests and marked them unflaky.
Also broke up project_mgmt into project_mgmt, testing just Projects CRUD, and project_rule_mgmt, testing just Rules CRUD.

This we have Cypress coverage for the Projects GUI section and most of the Project Rules flow.

What's missing is testing the project update in the UI. I'd like to make that it's own test, since it will involve a lot of moving parts: adding some resources to projects, loading some nodes that we'll write rules for, granting a new user project permissions and then having them log in to verify permissions. I've created a new tech debt card for that work.

To run them locally, follow the e2e Readme

👍 Definition of Done

project mgmt and project rule mgmt pass buildkite consistently

✅ Checklist

I have read the CONTRIBUTING document.
Tests added/updated?
Docs added/updated?
All commits have been signed-off for the Developer Certification of Origin.

no more flaky tests :))) Signed-off-by: Brenna Hewer-Darroch <[email protected]>

Signed-off-by: Brenna Hewer-Darroch <[email protected]>

now two tests, one for project mgmt and one for project rules mgmt Signed-off-by: Brenna Hewer-Darroch <[email protected]>

Signed-off-by: Brenna Hewer-Darroch <[email protected]>

srenatus

LGTM 👍

bcmdarroch · 2020-05-05T15:47:59Z

e2e/cypress/integration/ui/common/global_filter.spec.ts

@@ -58,24 +64,6 @@ describe('global projects filter', () => {
  });
 });

-function cleanupProjects(id_token: string): void {


this isn't needed since we have our handy cy.cleanupIAMObjectsByIDPrefixes (which also knows how to delete a project's rules before deleting it)

bcmdarroch · 2020-05-05T15:49:47Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts


 describe('project management', () => {
-  // we increase the default delay to mimic the average human's typing speed


decided to drop the typeDelay here. It was added as an early attempt to deflake the tests, since sometimes characters were missed when typing. but we've since found better ways to improve stability, mostly around adding expectations to ensure the page has loaded before interacting with elements.

bcmdarroch · 2020-05-05T15:50:30Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

+      cy.cleanupIAMObjectsByIDPrefixes(cypressPrefix, ['projects']);
+
+      // if there are projects that already exist, set the projects filter to all projects
+      cy.request({


might want to turn this into a helper, like clearProjectsFilter

if we're not doing it here, that would be a helpful comment within the code so as we duplicate the work it stands out

Looks like this is the only instance we're using it, so I'll leave this as is and updatethe comment

bcmdarroch · 2020-05-05T15:51:00Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

-  });
-
-  // something is occasionally resetting the projects filter to (unassigned)
-  itFlaky('can create a rule for the new project', () => {


moved these to project_rule_mgmt

bcmdarroch · 2020-05-05T15:51:45Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

    cy.contains(projectName).should('exist');
    cy.contains(projectID).should('exist');

    cy.url().should('include', '/settings/projects');
  });

-  it('addPolicies checkbox is always checked on modal open', () => {


this is now just an assertion in can create a project with a custom ID and generate associated policies

bcmdarroch · 2020-05-05T15:54:07Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

+    cy.wait('@getProjects');
+
+    cy.get('app-project-list chef-td').contains(customWithoutPolsProjectID).parent()
+      .find('.mat-select-trigger').as('controlMenu');


I always had trouble selecting the right element for the control menu in these tests and getting a successful interaction, but it looks like our mat-select refactor made it easier to target :)

bcmdarroch · 2020-05-05T15:54:39Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

      }
    });
  });
-
-  // these tests are currently interdependent so mark as flaky until the above isn't
-  itFlaky('can create a project with a custom ID', () => {


moved this up with the other create tests and changed the ID so we wouldn't run into conflicts

blakestier · 2020-05-05T16:16:47Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

+    associatedPolicies.forEach((policy) => {
+      cy.request({
+        auth: { bearer: adminIdToken },
+        url: `/apis/iam/v2/policies/${customWithoutPolsProjectID}-${policy}`,


this is sort of hard to parse at first glance. I wonder if there's a way to make it clearer?

maybe associatedPolicies -> appendedPolicyIds
policy -> policy_id

and or maybe create a new variable above the request instead of inside it with a new var name? Like autoCreatedPolicyId or something?

🤷

maybe i'm hung up on associated over autoGenerated? 🙃

went with a combo of yours and @msorens' suggestions

blakestier · 2020-05-05T16:19:59Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

+      ]
+    };
+    // add rule via the API to save time,
+    // since we test rules more thoroughly in project_rule_mgmt.spec.ts


blakestier · 2020-05-05T16:22:00Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

-    // we throw in a should so cypress retries until introspection allows menu to be shown
-    cy.get('@controlMenu').should('be.visible')
+    // we throw in a `should` so cypress retries until introspection allows menu to be shown
+    cy.get('@controlMenu').scrollIntoView().should('be.visible')


oooooh scroll into vieewwwww

yeah pretty slick 😎 might need to use that in some of our other tests

blakestier · 2020-05-05T16:23:11Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

+    cy.request({
+      auth: { bearer: adminIdToken },
+      method: 'DELETE',
+      url: `/apis/iam/v2/projects/${customWithoutPolsProjectID}/rules/${rule.id}`


Should we check for the "Project UPdate" banner?

(I know we don't want to test the functionality of the banner here, however, and we could remove when we add the fuller update tests)

blakestier · 2020-05-05T16:27:26Z

e2e/cypress/integration/ui/settings/project_rule_mgmt.spec.ts

+
+    cy.url().should('include', `/settings/projects/${project.id}`);
+    cy.get('app-project-details chef-td').contains(ruleID);
+    cy.get('app-pending-edits-bar').contains('Project edits pending');


is this the project update banner?

Yeah, I can add a comment

blakestier

Awesome. Thanks for adding new tests along the way! 🤗

I had some clarification questions and one idea about making this a big more readable-- I think all of your inline comments do loads to that end, though.

In general, I wonder if there's a way to make some of these expectations more readable (like

cy.get('app-project-details chef-td').contains(ruleID).parent()
      .find('.mat-select-trigger').as('controlMenu');

doesn't read in a way where I really know what is being verified 🤔) but it's a much bigger dilemma than this PR which fixes all the things and THEN makes new wonderful things.

msorens

Nice work!! Just a couple suggestions/questions.

msorens · 2020-05-05T16:07:27Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

-    cy.get('[data-cy=operator-dropdown]').select('member of');
-    cy.get('[data-cy=operator-dropdown]').select('equals');
-    cy.get('[data-cy=rule-value] input').type('Chef Policy Group Foo');
+    // don't create associated project policies


Helpful to provide a bit more context

Suggested change

// don't create associated project policies

// checkbox is selected by default; uncheck so we do NOT create associated project policies

msorens · 2020-05-05T16:19:28Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

+    cy.contains(customWithoutPolsProjectID).should('exist');
+
+    // verify no associated policies were generated
+    associatedPolicies.forEach((policy) => {


This confused me for a moment; would be a bit more clear with this:

Suggested change

associatedPolicies.forEach((policy) => {

associatedPolicySuffixes.forEach(suffix => {

msorens · 2020-05-05T16:36:12Z

e2e/cypress/integration/ui/settings/project_mgmt.spec.ts

+    // navigate back to the project list page and get latest project data
+    cy.route('GET', '/apis/iam/v2/projects').as('getProjects');
+    cy.get('.breadcrumb').click();


Did the CreateRule change the current page? I thought we were on the ListProjects page at the start of this test, but if you are clicking on a breadcrumb, then apparently not...?

Assuming on the project detail page, then, and the breadcrumb click takes us back to the ListProjects page, then why is the explicit projects fetch needed? Isn't that done by the breadcrumb click navigation?

The cy.route line isn't actually fetching the data, it specifies what API call Cypress should wait for on L220 (cy.wait('@getProjects');)

So we return to the list page with the breadcrumb, wait for the ListProjects call to come back, then proceed with the rest of the test.

Brenna Hewer-Darroch added 6 commits May 4, 2020 12:30

cypress: aspirational state

c5fb592

no more flaky tests :))) Signed-off-by: Brenna Hewer-Darroch <[email protected]>

(wip) get it passing locally

9431719

Signed-off-by: Brenna Hewer-Darroch <[email protected]>

finish stabilizing tests locally

45b89d1

Signed-off-by: Brenna Hewer-Darroch <[email protected]>

update notification selectors

0066fdb

Signed-off-by: Brenna Hewer-Darroch <[email protected]>

clean up projects after test run

6d85742

Signed-off-by: Brenna Hewer-Darroch <[email protected]>

split up project mgmt

a389f7c

now two tests, one for project mgmt and one for project rules mgmt Signed-off-by: Brenna Hewer-Darroch <[email protected]>

bcmdarroch force-pushed the bhd/2697/cypress_project_mgmt branch from 9819a32 to a389f7c Compare May 4, 2020 23:23

Brenna Hewer-Darroch added 2 commits May 4, 2020 17:05

add tests around generating associating policies

fb2f440

Signed-off-by: Brenna Hewer-Darroch <[email protected]>

add tests around deleting projects with rules

eae4185

Signed-off-by: Brenna Hewer-Darroch <[email protected]>

srenatus approved these changes May 5, 2020

View reviewed changes

bcmdarroch commented May 5, 2020

View reviewed changes

bcmdarroch marked this pull request as ready for review May 5, 2020 15:55

blakestier reviewed May 5, 2020

View reviewed changes

blakestier approved these changes May 5, 2020

View reviewed changes

msorens approved these changes May 5, 2020

View reviewed changes

bcmdarroch merged commit ba13b57 into master May 5, 2020

chef-expeditor bot deleted the bhd/2697/cypress_project_mgmt branch May 5, 2020 21:29

This was referenced Oct 30, 2022

[Snyk] Security upgrade karma from 4.1.0 to 6.0.0 ekmixon/automate#74

Open

[Snyk] Security upgrade karma from 0.13.22 to 6.0.0 ekmixon/automate#75

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[automate-2697] stabilize project mgmt tests #3584

[automate-2697] stabilize project mgmt tests #3584

bcmdarroch commented May 4, 2020 •

edited

Loading

srenatus left a comment

bcmdarroch May 5, 2020

bcmdarroch May 5, 2020

bcmdarroch May 5, 2020

blakestier May 5, 2020

bcmdarroch May 5, 2020

bcmdarroch May 5, 2020

bcmdarroch May 5, 2020

bcmdarroch May 5, 2020

bcmdarroch May 5, 2020

blakestier May 5, 2020

blakestier May 5, 2020

bcmdarroch May 5, 2020

blakestier May 5, 2020

blakestier May 5, 2020

bcmdarroch May 5, 2020

blakestier May 5, 2020

blakestier May 5, 2020

bcmdarroch May 5, 2020

blakestier May 5, 2020

bcmdarroch May 5, 2020

blakestier left a comment •

edited

Loading

msorens left a comment •

edited

Loading

msorens May 5, 2020

msorens May 5, 2020

msorens May 5, 2020

bcmdarroch May 5, 2020 •

edited

Loading


		describe('project management', () => {
		// we increase the default delay to mimic the average human's typing speed

	// don't create associated project policies
	// checkbox is selected by default; uncheck so we do NOT create associated project policies

	associatedPolicies.forEach((policy) => {
	associatedPolicySuffixes.forEach(suffix => {

[automate-2697] stabilize project mgmt tests #3584

[automate-2697] stabilize project mgmt tests #3584

Conversation

bcmdarroch commented May 4, 2020 • edited Loading

🔩 Description: What code changed, and why?

👍 Definition of Done

✅ Checklist

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakestier left a comment • edited Loading

Choose a reason for hiding this comment

msorens left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcmdarroch May 5, 2020 • edited Loading

Choose a reason for hiding this comment

bcmdarroch commented May 4, 2020 •

edited

Loading

blakestier left a comment •

edited

Loading

msorens left a comment •

edited

Loading

bcmdarroch May 5, 2020 •

edited

Loading