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

Batch coverage data sent to combineCoverage to prevent timeouts #877

Merged

Conversation

aantes-st
Copy link
Contributor

@aantes-st aantes-st commented Sep 9, 2024

Description

Some users, including myself, have experienced task timeouts when large coverage reports are sent to the Node process. This change

  • Adds support for environment variable sendCoverageBatchSize
  • Adds a support utility function for retrieving and validating the variable
  • Sends coverage to Node backend in batches of size sendCoverageBatchSize only if the environment variable is set and valid

This may address issue #455

Testing

In the existing full-report scenario, I was experiencing timeouts even when setting taskTimeout to 10+ minutes. With these changes, the coverage report is successfully constructed in seconds.

…keys as the batching mechanism. This is to prevent very large objects being sent across processes all at once. Objects of sufficient size have been causing timeouts during this transition.
@cypress-app-bot
Copy link

@aantes-st
Copy link
Contributor Author

@cacieprins The batching is currently hardcoded at every 500 keys, but this could be configurable, though that would require a lot more changes and is probably outside proper scope. Let me know what you think.

@cacieprins
Copy link
Contributor

@aantes-st Thank you for your contribution! I have some questions before tackling the configurability decision:

  • How did you arrive at the 500 mark?
  • Do you know which part of the underlying task was slow? I'm curious if we could improve performance there instead of batching arbitrarily.

Thank you!

@aantes-st
Copy link
Contributor Author

aantes-st commented Sep 25, 2024

@aantes-st Thank you for your contribution! I have some questions before tackling the configurability decision:

  • How did you arrive at the 500 mark?
  • Do you know which part of the underlying task was slow? I'm curious if we could improve performance there instead of batching arbitrarily.

Thank you!

@cacieprins the 500 mark is arbitrarily chosen - there's no significance behind it. The performance issue seems to be in the transfer between support and task, where the coverage string is sent to the Node process. At first I thought it was due to the stringify and decode but after adding some debug statements, I found that the task handler never starts processing, indicating that the string never arrived.

@cacieprins
Copy link
Contributor

@aantes-st That makes sense!

I think it would be best to make this configurable, however. Since the configuration value needs to be available in the browser, it can be attached to the env section of Cypress config, and accessed fairly easily in the command. I would recommend not batching if the env is not present.

How does that sound to you?

@aantes-st
Copy link
Contributor Author

@aantes-st That makes sense!

I think it would be best to make this configurable, however. Since the configuration value needs to be available in the browser, it can be attached to the env section of Cypress config, and accessed fairly easily in the command. I would recommend not batching if the env is not present.

How does that sound to you?

@cacieprins sounds good! I'll get those changes committed shortly.

@aantes-st
Copy link
Contributor Author

@cacieprins see the latest commits. If those look good, I will update the README file with information about using the new variable-based configuration.

@cacieprins
Copy link
Contributor

Looks good to me!

@aantes-st
Copy link
Contributor Author

Looks good to me!

@cacieprins Added README info about the environment variable and it's use. Should be all set!

cacieprins
cacieprins previously approved these changes Oct 2, 2024
@cacieprins cacieprins dismissed their stale review October 2, 2024 15:04

Test investigation

Copy link
Contributor

@cacieprins cacieprins left a comment

Choose a reason for hiding this comment

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

Sorry for the premature approval, @aantes-st --

Can you add a test case to the test-apps dir that includes this environment variable? This test case should be constructed such that the batch logic is executed. Completely viable to set the env to a very low value to ensure that things are batched.

The tests in this project assert that coverage is 100% of the files that should be analyzed for coverage.

Once the test app is added, it needs to be included in the circle yml, both in the jobs matrix - https://github.com/cypress-io/code-coverage/blob/master/.circleci/config.yml#L146 and the requires list - https://github.com/cypress-io/code-coverage/blob/master/.circleci/config.yml#L172

Thank you!

@aantes-st
Copy link
Contributor Author

@cacieprins I've added the test-app. I am not sure if/how the outcome is asserted (the check-coverage script?), but I did validate that all files are present in the coverage report output.

@jennifer-shehane jennifer-shehane merged commit a5a95d5 into cypress-io:master Oct 2, 2024
25 of 26 checks passed
@cypress-app-bot
Copy link

🎉 This PR is included in version 3.13.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants