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

Add kubewarden spec to extension compatibility tests #11668

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

aalves08
Copy link
Member

@aalves08 aalves08 commented Aug 19, 2024

Summary

Fixes #11482
Fixes #9232

Occurred changes and/or fixed issues

  • Add spec for Kubewarden e2e extension compatibility tests

Technical notes summary

Based on the work done in #11626, which needed a profound rebase from master which spawned this new PR.

Areas or cases that should be tested

Areas which could experience regressions

Screenshot/Video

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

@aalves08 aalves08 self-assigned this Aug 19, 2024
@aalves08 aalves08 added this to the v2.10.0 milestone Aug 19, 2024
@aalves08 aalves08 force-pushed the 11482-add-kubewarden-spec branch from a4c688d to 97aaa24 Compare August 19, 2024 17:57
@aalves08 aalves08 marked this pull request as ready for review August 20, 2024 16:46
@aalves08
Copy link
Member Author

@richard-cox @jordojordo the usual cleanup is still needed because I wanted to demonstrate that the spec is working just fine.

@jordojordo I think we've covered what we discussed in terms of meaningful tests, right?

  • create an admission policy, trigger it and verify it in a compliance report
  • create a policy server via YAML

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

general approach looks good. there's a few POs that could wrap some existing. we might want to spin out some of the custom POs from the utils file at some point (if it grows)

cypress/e2e/po/edit/ingress.po.ts Outdated Show resolved Hide resolved
cypress/e2e/po/pages/explorer/cronjob.po.ts Outdated Show resolved Hide resolved
super(ProjectNamespaceDetailPagePo.createPath(clusterId, id));
}

clickComplianceTab() {
Copy link
Member

Choose a reason for hiding this comment

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

these two are specific to kw

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've created a new class in kubewarden.utils called KubewardenResourceComplianceReportPagePo which is the same for any resource, afaik, and then imported it in projectNamespaceDetail to be exposed as kubewardenComplianceReport, which will access the methods.

I think the context is correct, to keep it accessible in projectsNamespaces, but the "ownership" I think you are right. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

The aim is to keep kubewarden specific code out of generic code.

I might be wrong, but if the compliance tab is an kw specific additional tab in the detail page of a project. so suggest the hierarchy as

KubewardenResourceComplianceReportTabPo extends ComponentPo {
  complianceSortableTable() {
    return new SortableTablePo(this.self().get('#policy-report-tab .sortable-table'));
  }
}

KubewardenProjectNamespaceDetailPagePo extends ProjectNamespaceDetailPagePo {
  clickComplianceTab() {
    return new TabbedPo().clickTabWithSelector('[data-testid="btn-policy-report-tab"]');
  }

  complianceTab() {
     return new KubewardenResourceComplianceReportTabPo(this.self())
  }
}

So there's a KW specific project namespace detail page, inheriting all the root page functionality, exposing new KW specific functionality

cypress/e2e/po/edit/ingress.po.ts Show resolved Hide resolved
cypress/e2e/po/extensions/kubewarden/kubewarden.utils.ts Outdated Show resolved Hide resolved
cypress/e2e/po/extensions/kubewarden/kubewarden.utils.ts Outdated Show resolved Hide resolved
@aalves08 aalves08 requested a review from richard-cox August 22, 2024 17:04
@aalves08
Copy link
Member Author

@richard-cox did a few changes based on your comments, but since it generally looks good, tomorrow I am going to base my work on neuvector spec on this PR. Thank you for your time looking into this! 🙏

Copy link
Member

@jordojordo jordojordo left a comment

Choose a reason for hiding this comment

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

LGTM - small comment about the naming scheme, and I think those gates are ready to be removed.

.github/workflows/check-plugins.yaml Show resolved Hide resolved
cypress/e2e/po/extensions/kubewarden/kubewarden.utils.ts Outdated Show resolved Hide resolved
@aalves08 aalves08 force-pushed the 11482-add-kubewarden-spec branch from 8c21d28 to d5db1c2 Compare August 28, 2024 15:10
@aalves08 aalves08 requested a review from jordojordo August 28, 2024 15:11
super(ProjectNamespaceDetailPagePo.createPath(clusterId, id));
}

kubewardenComplianceReport() {
Copy link
Member

Choose a reason for hiding this comment

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

This would be rmeoved as part of #11668 (comment)


successfulCountCheck(countExpected: number, timeout = 15000) {
// let's give it a buffer for the job to finish running
cy.wait(timeout); // eslint-disable-line cypress/no-unnecessary-waiting
Copy link
Member

Choose a reason for hiding this comment

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

you should be able to extend the time get allows for an element to appear by passing in args to cy.get (check out usages of MEDIUM_TIMEOUT_OPT, LONG_TIMEOUT_OPT, EXTRA_LONG_TIMEOUT_OPT)

expect(response?.body.metadata).to.have.property('namespace', namespaceToCheck);
}

cy.wait(5000); // eslint-disable-line cypress/no-unnecessary-waiting
Copy link
Member

Choose a reason for hiding this comment

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

why is there a 5 second wait here?

// let's give it a buffer for the job to finish running
cy.wait(timeout); // eslint-disable-line cypress/no-unnecessary-waiting

return cy.get('.gauges__pods div:nth-child(1) div h1').invoke('text').should('contain', countExpected);
Copy link
Member

Choose a reason for hiding this comment

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

Feels like this should be a CountGauge PO

expect(response?.statusCode).to.eq(201);
expect(response?.body.metadata).to.have.property('name', nameToCheck);

cy.wait(5000); // eslint-disable-line cypress/no-unnecessary-waiting
Copy link
Member

Choose a reason for hiding this comment

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

why's there a wait here?


extensionsPo.addExtensionsRepositoryDirectLink(EXTENSION_REPO, EXTENSION_BRANCH, EXTENSION_CLUSTER_REPO_NAME, true);
// let's wait a bit so that the repo is available in extensions screen
cy.wait(10000); // eslint-disable-line cypress/no-unnecessary-waiting
Copy link
Member

Choose a reason for hiding this comment

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

this should be achievable with a should or get with timeout

kubewardenPo.chartInstallPage().waitForChartPage('rancher-charts', 'rancher-monitoring');
// monitoring chart install page takes a bit to come up. The search for "rancherMonitoringInstallIntoProjectSelect"
// might fail because of this. Let's give it a time buffer here
cy.wait(10000); // eslint-disable-line cypress/no-unnecessary-waiting
Copy link
Member

Choose a reason for hiding this comment

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

there should be other ways to check for screen content is available, applies to all the cy.wait(10000)'s on page

@nwmac nwmac modified the milestones: v2.10.0, v2.11.0, v2.12.0 Oct 29, 2024
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.

Extensions: Vue 3 extension validation and compatibility E2E Tests for Kubewarden
4 participants