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

Integrate e2e-test-cycloud job with Buildmaster #260

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rgagarinr
Copy link
Contributor

Issue: FEI-6125

Summary:
This draft PR introduces a Cypress Cloud job that runs in parallel with the existing Lambda e2e-test job. It also includes a Mocha implementation for each worker. (Note: The Mocha implementation requires updates with yarn, which can be addressed in a subsequent PR.)

The primary goal is to utilize Jenkins stash functionality to capture and merge all junit reports into a single file, enabling streamlined processing.

@rgagarinr rgagarinr requested a review from a team as a code owner January 22, 2025 19:23

dir('webapp/services/static') {
// We build the secrets first, so that we can create test users in our
// tests.
sh("yarn cypress:secrets");
exec(runE2ETestsArgs);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is stashing of individual worker results stash includes: "${resultsDir}/junit-${workerId}.xml", name: "worker-${workerId}-results";

Copy link
Member

Choose a reason for hiding this comment

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

Are you planning on doing that in this PR, or a follow-up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the confusion, but I think yes, we will work on the junit implementation after this PR.

"run-lambdatest": { runLambdaTest(); },
"test-cycloud": { runCypressCloud(); },
]);
// runLambdaTest();
}

stage("Analyzing results") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

// Collect results from the triggered job
        stage("Collect Cypress Results") {
            def resultsDir = "results";
            def mergedResults = "${resultsDir}/junit-merged.xml";

            sh("mkdir -p ${resultsDir}");

            // Unstash results from all workers
            for (int i = 0; i < params.NUM_WORKER_MACHINES.toInteger(); i++) {
                unstash name: "worker-${i}-results";
            }

            // Merge results into a single JUnit XML file
            sh("""
                find ${resultsDir} -name '*.xml' -print0 | xargs -0 cat > ${mergedResults}
            """);

            echo("Merged results written to ${mergedResults}");
        }

Copy link
Member

Choose a reason for hiding this comment

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

Is this how results get merged for unit tests too? If not, what's different and why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it’s a slightly different alternative way of how we might collect junit results - maybe more straightforward.


dir('webapp/services/static') {
// We build the secrets first, so that we can create test users in our
// tests.
sh("yarn cypress:secrets");
Copy link
Member

Choose a reason for hiding this comment

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

Cc @jeresig -- is this something we need to be doing some other way, now?

Copy link
Member

Choose a reason for hiding this comment

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

Yes - this will need to use the new wrapper script I added here: https://github.com/Khan/webapp/pull/28829 good catch! I'm going to be landing the script to master today.

Copy link
Member

@somewhatabstract somewhatabstract Jan 22, 2025

Choose a reason for hiding this comment

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

We don't need to call yarn cypress:secrets at all. Secrets are handled internally to the CyCloud runner and passed as environment variables option, rather than a JSON file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes sense we can remove this yarn secrets call.

Copy link
Member

@csilvers csilvers left a comment

Choose a reason for hiding this comment

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

This seems pretty safe to start off with, I appreciate you being careful about the new job not interfering with the existing job. I want to make sure I understand about the rm first before approving though.

@@ -166,15 +166,25 @@ def runAllTestClients() {
def runE2ETests(workerId) {
Copy link
Member

Choose a reason for hiding this comment

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

What directory are we in when this function is run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I’m not mistaken, the script executes the runE2ETests function within the webapp/services/static directory. This is defined in the dir('webapp/services/static') block in the parent function (runAllTestClients).

def resultsDir = "results"; // Directory to store results
def junitFile = "${resultsDir}/junit-${workerId}.xml";

// Ensure the results directory exists
Copy link
Member

Choose a reason for hiding this comment

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

Don't you also need to clear out any old xml files that might be there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks! Yeah, we need to clean up the old xml files first to avoid conflicts.


dir('webapp/services/static') {
// We build the secrets first, so that we can create test users in our
// tests.
sh("yarn cypress:secrets");
exec(runE2ETestsArgs);
}
Copy link
Member

Choose a reason for hiding this comment

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

Are you planning on doing that in this PR, or a follow-up PR?

Comment on lines +178 to +181
"./dev/cypress/e2e/tools/start-cypress-cloud-run.ts",
"--cyConfig baseUrl=\"${E2E_URL}\"",
"--reporter mocha-junit-reporter",
"--reporter-options mochaFile=${junitFile}"
Copy link
Member

Choose a reason for hiding this comment

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

note: This invocation will need to change. These args aren't supported by the runner and I'm not sure we need to provide them like this. I think most of this can be calculated internally to the code.

Copy link
Member

Choose a reason for hiding this comment

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

In addition, we're not going to do junit right away. That's a stretch goal once we have things working.

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.

4 participants