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.

Sign up for GitHub

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
Open

Integrate e2e-test-cycloud job with Buildmaster #260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions jobs/e2e-test-cycloud.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Copy link
Member

Choose a reason for hiding this comment

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

OK, you don't want to create the tmpdir in webapp/services/static/results -- you want it to be at the top level. According to my web research, you can do:

resultsDir = "${WORKSPACE}/results"; // Directory to store results

but you'll have to test that.

Also, "results" is a very generic name. How about something like "junit-output/"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Craig! {WORKSPACE} works, yeah and I’ll rename results! cc @michaelmendoza42

echo("Starting e2e tests for worker ${workerId}");

// Determine which environment we're running against, so we can provide a tag
// in the LambdaTest build.
// Define which environment we're running against, and setting up junit report
def e2eEnv = E2E_URL == "https://www.khanacademy.org" ? "prod" : "preprod";

// NOTE: We hard-code the values here as it is an experiment that will be
// removed soon.
def runE2ETestsArgs = ["env", "CYPRESS_PROJECT_ID=2c1iwj", "CYPRESS_RECORD_KEY=4c530cf3-79e5-44b5-aedb-f6f017f38cb5", "./dev/cypress/e2e/tools/start-cypress-cloud-run.ts"];
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.

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

def runE2ETestsArgs = [
"./dev/cypress/e2e/tools/start-cypress-cloud-run.ts",
"--cyConfig baseUrl=\"${E2E_URL}\"",
"--reporter mocha-junit-reporter",
"--reporter-options mochaFile=${junitFile}"
Comment on lines +178 to +181
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.

];

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 •
edited
Loading

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.

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.

}
Expand Down
27 changes: 26 additions & 1 deletion jobs/e2e-test.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,27 @@ def runLambdaTest() {
}
}

def runCypressCloud(){
build(job: 'e2e-test-cycloud',
parameters: [
string(name: 'SLACK_CHANNEL', value: "#cypress-testing"),
string(name: 'REVISION_DESCRIPTION', value: REVISION_DESCRIPTION),
string(name: 'DEPLOYER_USERNAME', value: params.DEPLOYER_USERNAME),
string(name: 'URL', value: E2E_URL),
string(name: 'NUM_WORKER_MACHINES', value: params.NUM_WORKER_MACHINES),
string(name: 'TEST_RETRIES', value: "1"),
// It takes about 5 minutes to run all the Cypress e2e tests when
// using the default of 20 workers. This build is running in parallel
// with runTests(). During this test run we don't want to disturb our
// mainstream e2e pipeline, so set propagate to false.
],
propagate: false,
// The pipeline will NOT wait for this job to complete to avoid
// blocking the main pipeline (runLambdaTest).
wait: false,
);
}

// This method filters a common 'DEPLOYER_USERNAME' into a series of comma
// seperated slack user id's. For Example:
// DEPLOYER_USERNAME: <@UMZGEUH09> (cc <@UN5UC0EM6>)
Expand Down Expand Up @@ -338,7 +359,11 @@ onWorker(WORKER_TYPE, '5h') { // timeout
// tests failed. That is, this will succeed as long as there
// are no lambdatest framework errors; it's up to use to look
// for actual smoketest-code errors in analyzeResults(), below.
runLambdaTest();
parallel([
"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.

Expand Down