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

Add support for TEST_SKIP_AFTER_FAILURE_COUNT #5475

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

tdiesler
Copy link
Contributor

@tdiesler tdiesler commented May 8, 2024

Borrowed from maven surefire - count test failures and skip subsequent tests when failure count > $TEST_SKIP_AFTER_FAILURE_COUNT
The default is TEST_SKIP_AFTER_FAILURE_COUNT=3

This can significantly speed up validation of bad PRs

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

Not sure to understand this change request. Consider that when you're running in GH actions, each workflow would execute each test suite individually. For instance, it does make test-common. So, in such case, this new variable is never took in consideration. The only case where it can make sense here would be in the test-smoke. However, such a test is only executed at night time before nightly release, and, it only have 4 possible failures, so, we'd really skip almost nothing.

@tdiesler
Copy link
Contributor Author

tdiesler commented May 8, 2024

make test-smoke takes > 30min on my box. If something goes wrong at the beginning, it already safes quite some time when set to 1. We can expand this to the other test suites later, if wanted

@squakez
Copy link
Contributor

squakez commented May 8, 2024

make test-smoke takes > 30min on my box. If something goes wrong at the beginning, it already safes quite some time when set to 1. We can expand this to the other test suites later, if wanted

Okey, then, we better make the variable scope local to this specific action. Otherwise we can get mislead and think that this is a global scope and would affect all test suite executions.

@christophd
Copy link
Contributor

How about a TEST_FAIL_FAST=true option so the test suite stops after the 1st failure. I don't see why somebody would be interested in stopping execution after >1 failing tests when asking for fast feedback.

@tdiesler
Copy link
Contributor Author

tdiesler commented May 8, 2024

In surefire, it defaults to 0. Folks set it higher to see e.g. "the first three failures"
Would you like to expand this to the other test suites i.e. if smoke fails, no need to execute the others?

@tdiesler tdiesler force-pushed the skip_after_failure branch 2 times, most recently from 819e3a1 to bb9f601 Compare May 9, 2024 07:02
@tdiesler tdiesler force-pushed the skip_after_failure branch 2 times, most recently from 0b6ee70 to d9d91f2 Compare May 24, 2024 07:59
@tdiesler
Copy link
Contributor Author

tdiesler commented Jun 3, 2024

Ready to merge

@squakez
Copy link
Contributor

squakez commented Jun 3, 2024

Ready to merge

Please, see this comment #5475 (comment) - if we want this variable, then, we need to make it in the specific test scope to avoid users to mislead its meaning. Thanks.

@christophd
Copy link
Contributor

just another nitpick: I have started to document the available E2E test env vars in https://github.com/apache/camel-k/blob/main/e2e/README.md#environment-variables

Maybe we can add a section on how to run tests locally with Makefile and the possible env var settings

@tdiesler
Copy link
Contributor Author

tdiesler commented Jun 4, 2024

Okey, then, we better make the variable scope local to this specific action. Otherwise we can get mislead and think that this is a global scope and would affect all test suite executions.

This is done

@tdiesler
Copy link
Contributor Author

tdiesler commented Jun 4, 2024

just another nitpick: I have started to document the available E2E test env vars in https://github.com/apache/camel-k/blob/main/e2e/README.md#environment-variables

Maybe we can add a section on how to run tests locally with Makefile and the possible env var settings

I now promoted the env var table to the top and the skip var is now local to the smoke tests like this ...

test-smoke: do-build
	TEST_SKIP_AFTER_FAILURE_COUNT="$(TEST_SKIP_AFTER_FAILURE_COUNT)"; \
	if [[ $$TEST_SKIP_AFTER_FAILURE_COUNT = "" ]]; then \
		TEST_SKIP_AFTER_FAILURE_COUNT=0; \
	fi; \
	FAILED=0; STAGING_RUNTIME_REPO="$(STAGING_RUNTIME_REPO)"; \

Folks wanting to see more than one smoke test failure are likely familiar with the Makefile anyway - so I guess it sufficiently documents itself here.

@tdiesler
Copy link
Contributor Author

tdiesler commented Jun 4, 2024

could we pls run this again?

@tdiesler
Copy link
Contributor Author

tdiesler commented Jun 4, 2024

fails unrelated, pls try again

@tdiesler
Copy link
Contributor Author

tdiesler commented Jun 4, 2024

repeatedly fails in

❌ TestOLMOperatorUpgrade (16m29.34s)
❌ TestOLMOperatorUpgrade/OLM_upgrade (10m0.03s)

@squakez squakez merged commit df2c5d4 into apache:main Jun 4, 2024
11 of 12 checks passed
@squakez
Copy link
Contributor

squakez commented Jun 4, 2024

Yes, the checks were failing because it required a rebase after 2.3.2 release. Merging anyway.

@tdiesler tdiesler deleted the skip_after_failure branch June 4, 2024 14:45
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.

None yet

3 participants