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

ci: terminate slow tests #691

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

Conversation

huancheng-trili
Copy link
Collaborator

@huancheng-trili huancheng-trili commented Nov 27, 2024

Context

Closes JSTZ-208.

JSTZ-208

Description

Terminate tests that take too much time in CI to save resources and prevent unexpected long-running tests from blocking the CI runner.

Manually testing the PR

A test branch contains changes in this PR and a dummy test that does nothing but sleeps for 2 minutes. Workflow run: https://github.com/jstz-dev/jstz/actions/runs/12035957074/job/33556133877

The test was terminated and the overall check failed as expected.

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.85%. Comparing base (5fbe97d) to head (c5e6d9f).


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fbe97d...c5e6d9f. Read the comment docs.

@huancheng-trili huancheng-trili marked this pull request as ready for review November 27, 2024 12:35
@huancheng-trili huancheng-trili removed the request for review from zcabter November 27, 2024 12:37
@johnyob johnyob mentioned this pull request Nov 27, 2024
@@ -135,7 +135,7 @@ in {
# Don't run the `jstz_api` integration tests until they've been paralellized
#
# Note: --workspace is required for --exclude. Once --exclude is removed, remove --workspace
cargoNextestExtraArgs = "--workspace --test \"*\" --exclude \"jstz_api\"";
cargoNextestExtraArgs = "--workspace --test \"*\" --exclude \"jstz_api\" --config-file ${src}/.config/nextest.toml";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this be needed for the cargo-llvm-cov check as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at cargo llvm-cov test the option isn't available, but it is for cargo llvm-cov nextest. #658 changes the command to nextest -- though this could be split out/included in this PR to avoid PR dependencies

Copy link
Collaborator Author

@huancheng-trili huancheng-trili Nov 28, 2024

Choose a reason for hiding this comment

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

I see. Do you want to split that nextest change into an individual PR? You can apply config-file in that PR after this one is merged or I can wait for your PRs and patch cargo-llvm-cov in this PR.

@johnyob johnyob assigned huancheng-trili and unassigned johnyob Nov 27, 2024
Copy link
Collaborator

@johnyob johnyob left a comment

Choose a reason for hiding this comment

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

See comment about cargo-llvm-cov

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.

2 participants