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

Generate PDF report for cypress + environment management improvements #1079

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

Conversation

roicarrera
Copy link
Contributor

added mochawesome
added pdf-generator script file
read baseurl on config maps based on environment
added skip for prod

@cschweikert
Copy link
Member

Hi @roicarrera, since this PR is also reworking parts of the respective configs, I would propose to use cypress.config.ts as base configuration that is then mixed into the other configs (import the base config and using the spread operator). This would prevent/remove a lot of code duplication and put more focus on the actual difference between those files.

e2e-cypress/files/cypress-acceptance.config.ts Outdated Show resolved Hide resolved
e2e-cypress/files/cypress-integration.config.ts Outdated Show resolved Hide resolved
e2e-cypress/files/package.json Show resolved Hide resolved
e2e-cypress/files/pdf-generator.ts Outdated Show resolved Hide resolved
e2e-cypress/files/pdf-generator.ts Outdated Show resolved Hide resolved
e2e-cypress/files/support/test-evidence.ts Show resolved Hide resolved
e2e-cypress/Jenkinsfile.template Outdated Show resolved Hide resolved
reworked config files to be clearer
fixed typo in reporterOptions
added generate:pdf script to package.json
moved folder creation
Copy link
Member

@cschweikert cschweikert left a comment

Choose a reason for hiding this comment

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

Thanks for making all the changes. Really improved things around the configs! Only little things left.

e2e-cypress/files/support/e2e.ts Outdated Show resolved Hide resolved
e2e-cypress/files/pdf-generator.ts Outdated Show resolved Hide resolved
e2e-cypress/files/support/test-evidence.ts Outdated Show resolved Hide resolved
Copy link
Member

@cschweikert cschweikert left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@metmajer metmajer left a comment

Choose a reason for hiding this comment

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

Hello @roicarrera,

  • We need a clear concept for generating reports, screenshots, and videos from our testing Quickstarters. I cannot see clearly how their creation and storing does not affect Jenkins. The only place to store potentially larger files is Nexus.
  • Disabling test execution in PROD in the way you did seriously messes with regulations. A solid design involving the core platform team needs to be done to prevent potential damage.

As a general rule, I also suggest creating one PR for one purpose, and not mixing many different things. This will help increase clarity during code reviews.

// including potential downtime, data corruption, or exposure of sensitive information.
// This block is designed to skip tests in the production environment to avoid these risks.
// If you choose to enable testing in production, do so at your own risk and take all necessary precautions.
if (context.environment == 'prod') {
Copy link
Member

Choose a reason for hiding this comment

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

@roicarrera the Cypress Quickstarter is of type ods-test, which must support acceptance, integration, and installation tests. You cannot simply omit all tests in production. Since installation tests are mandatory by LeVA in the PROD environment, you are right now messing with compliance regulations. Changing the current testing behavior in PROD needs a clear design in agreemtn with the core platform team and consistency across all ods-test Quickstarters. FYI @BraisVQ

Copy link
Member

@metmajer metmajer Nov 12, 2024

Choose a reason for hiding this comment

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

@roicarrera your comment says that testing in production means to take a risk and requires precaution. What do you mean? We made this conciously to ensure that applications, once in production, work as expected although tests are not strictly required. Limiting test execution to installation tests is a possible and helpful improvement for the future for sure.

@@ -49,27 +60,32 @@ odsComponentPipeline(

def stageTest(def context) {
stage('Integration Test') {
// OPTIONAL: load environment variables for Azure SSO with MSALv2; please adapt variable names to your OpenShift config
// Define your DEV and QA base URLs in a config map in OpenShift; please adapt variable names to your OpenShift config
Copy link
Member

Choose a reason for hiding this comment

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

@roicarrera why define the DEV and QA base urls in a secret if 12-factor apps teaches us to provide them through the environment? I believe that ODS already does that. Then, a test case should only reference a path and resouce below the BASE URL. Can you clarify the design? What about PROD?

Comment on lines +98 to 111
sh 'npm run generate:pdf'
zip zipFile: 'cypress/pdf.zip', archive: false, dir: 'build/test-results/mochawesome/pdf'
archiveArtifacts artifacts: 'cypress/pdf.zip', fingerprint: true, daysToKeep: 2, numToKeep: 3

if (fileExists('cypress/videos')) {
zip zipFile: 'cypress/videos.zip', archive: false, dir: 'cypress/videos'
stash(name: "acceptance-test-videos-${context.componentId}-${context.buildNumber}", includes: 'cypress/videos.zip', allowEmpty: true)
archiveArtifacts artifacts: 'cypress/videos.zip', fingerprint: true, daysToKeep: 2, numToKeep: 3
}

if (fileExists('cypress/screenshots')) {
zip zipFile: 'cypress/screenshots.zip', archive: false, dir: 'cypress/screenshots'
if (fileExists('build/test-results/screenshots')) {
zip zipFile: 'cypress/screenshots.zip', archive: false, dir: 'build/test-results/screenshots'
stash(name: "acceptance-test-screenshots-${context.componentId}-${context.buildNumber}", includes: 'cypress/screenshots.zip', allowEmpty: true)
archiveArtifacts artifacts: 'cypress/screenshots.zip', fingerprint: true, daysToKeep: 2, numToKeep: 3
Copy link
Member

Choose a reason for hiding this comment

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

@roicarrera what's the intended purpose of mochawesome reports opposed to LeVA-based documentation? Why archive reports, videos, and screenshots in Jenkins where they keep consuming disk space, even only for two days, which could cause Jenkins to fail when the disk runs full?

Comment on lines +76 to +77
// cypressUser = sh(returnStdout: true, script:"oc get secret e2euser -o jsonpath='{.data.${context.environment}_username}' | base64 -d")
// cypressPassword = sh(returnStdout: true, script:"oc get secret e2euser -o jsonpath='{.data.${context.environment}_password}' | base64 -d")
Copy link
Member

Choose a reason for hiding this comment

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

@roicarrera credentials (secrets of type username and password) need to be consumed in a safe way so that they do not leak into the log stream. Please use Jenkins' withCredentials task. You can refer to other Quickstarters to see its use.

This was referenced Nov 12, 2024
@cschweikert
Copy link
Member

* We need a clear concept for generating reports, screenshots, and videos from our testing Quickstarters. I cannot see clearly how their creation and storing does not affect Jenkins. The only place to store potentially larger files is Nexus.

Build artifacts like screenshots and videos also fit the purpose of analyzing problems in case of broken tests. Yes, you should assure that e2e tests are successful BEFORE you commit. This could be done by running them on a locally running representation of your target system. However, the situation on the cluster is different. In rare cases you are even facing things like flaky tests that are hard to reproduce even on the cluster. In those situations as a developer you are thankful for every piece of information you could get.

In this context they fit where they are. However, we should discuss about the right amount of "daysToKeep". Also it would be great to understand, if storing screenshots and videos temporarily even creates a problem regarding storage sizes. Maybe Jenkins already takes care of this.

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.

3 participants