don't run serial tests any more when FOCUS is specified

[jim-minter]

follow up from #16842, may also solve the same issue as #16806

[jim-minter]

@smarterclayton @bparees @miminar let's try this?

[bparees]

we can't do this by itself, it means our nightly extended test jobs won't run the [Serial] tests. (we use focus to run all the [builds] and [image_ecosystem] tests and some of them are marked [Serial])

if there's a plan for how $FOCUS + [Serial] tests are going to be run, we'll need to include it in this PR.

[smarterclayton]

Not supported. Find a different solution. The only serial tests should be those that need while cluster access. Are you thinking of disruptive tests?

/lgtm

[smarterclayton]

"Whole"

[openshift-merge-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jim-minter, smarterclayton

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• test/extended/OWNERS [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[smarterclayton]

Or maybe slow tests?

[bparees]

i'm thinking of tests like our build test that confirms that the next build is started immediately after the last one. When it's run on an overloaded parallel environment, the timings become impossible to confirm. When it's run serially, it's rock solid and we ensure that we have not regressed the "next build starts when the previous build finishes" behavior.

And merging a PR that immediately disables existing tests strikes me as the wrong answer and is just asking for regressions.

This also makes it impossible run those tests locally, which is kinda the whole point of FOCUS in the first place.

At a minimum, anything currently marked [Serial] should be marked conformance so they continue to get run until we have an actual solution to this problem.

[bparees]

i'm also reasonably sure this behavior is going to confuse the heck out of someone in the future when they attempt to do a FOCUS on a [Serial] test and it won't run.

[bparees]

talked with @smarterclayton about this some more, sounds like he is going to create a serial bucket that runs all the serial tests. I can live w/ that, though i still think this will confuse people trying to run individual serial tests locally via FOCUS.

My other suggestion would be to run with something like --ginkgo.focus=$FOCUS.*?[Serial]|[Serial].*?$FOCUS for the serial portion. It won't work with 100% of regexes people might use, but it should work for most of them.

[miminar]

i'm also reasonably sure this behavior is going to confuse the heck out of someone in the future when they attempt to do a FOCUS on a [Serial] test and it won't run.

👍

This renders most of the tests in extended_image_registry job untestable - both locally and in CI.

Not supported. Find a different solution. The only serial tests should be those that need while cluster access. Are you thinking of disruptive tests?

In case of serial registry tests - several tests apply different registry configuration (read-only on/off acceptschema2 on/off etc.), which restarts the registry. Such tests cannot be run in parallel with other tests interacting with registry.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-merge-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[smarterclayton]

I'd rather not have that complexity. Avoid making tests serial, and maybe we'll add a serial job to PR that people can focus. On Oct 12, 2017, at 8:19 PM, Ben Parees <[email protected]> wrote: talked with @smarterclayton <https://github.com/smarterclayton> about this some more, sounds like he is going to create a serial bucket that runs all the serial tests. I can live w/ that, though i still think this will confuse people trying to run individual serial tests locally via FOCUS. My other suggestion would be to run with something like --ginkgo.focus=$FOCUS.*?[Serial]|[Serial].*?$FOCUS for the serial portion. It won't work with 100% of regexes people might use, but it should work for most of them. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16846 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p9_FXq_W2y7RYG8Z14MzeA9V8UlYks5srqyWgaJpZM4P3gQ8> .

[openshift-ci-robot]

@jim-minter: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_install_update	3b7138c	link	/test extended_conformance_install_update

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[bparees]

I'd rather not have that complexity. Avoid making tests serial, and maybe we'll add a serial job to PR that people can focus.

Seems like @miminar gave a pretty good reason for needing serial tests and prefixing/appending [Serial] does not seem that complex, especially compared to the other things that are being done to run our extended tests.

[bparees]

/hold
(we can't just disable all the registry tests w/o a plan)

[miminar]

(we can't just disable all the registry tests w/o a plan)

We've just discussed this with @Kargakis. We can provide a custom script for this particular job in here: https://github.com/openshift/aos-cd-jobs/blob/master/sjb/config/test_cases/test_pull_request_origin_extended_image_registry.yml
that would invoke ginkgo directly to test exactly what we need.

[bparees]

We've just discussed this with @Kargakis. We can provide a custom script for this particular job in here: https://github.com/openshift/aos-cd-jobs/blob/master/sjb/config/test_cases/test_pull_request_origin_extended_image_registry.yml
that would invoke ginkgo directly to test exactly what we need.

still means it'll be harder to run the tests locally (you'll have to invoke ginkgo manually there too) but sure.

[miminar]

still means it'll be harder to run the tests locally (you'll have to invoke ginkgo manually there too) but sure.

Yes, it's a workaround until the extended scripts will be generic enough to select what needs to be run and what not.

[miminar]

We can provide a custom script for this particular job in here

Initial untested version is here: openshift-eng/aos-cd-jobs#729

[smarterclayton]

Let's just create a serial pr job that supports focus. Let's stop hacking around bad decisions we made years ago.

Also, any serial registry test should be installing its own registry and hacking that. Updating the core registry is a bad pattern. I'm not aware of any reason why you can't spin up the registry image and point it as openshift. Should work fine.

[bparees]

Let's just create a serial pr job that supports focus. Let's stop hacking around bad decisions we made years ago.

how do you propose to do that? is that not still going to be ginkgo.focus="$FOCUS.*[Serial]" ?

The whole challenge here is selecting the set of tests that contain both [Serial] and $FOCUS.

[stevekuznetsov]

+100 to Ben's regex solution because it's simple and unblocks this. the underlying issues are:

• if your test is serial you're doing something sub-optimally (not starting your own registry to test against, not validating controller logic with fake event stream etc
• we should not try to tackle the test selection problem in bash
• this "organic" script is out of hand

We should spend the time to tackle these top-down (change Ginkgo? remove serial tests?) but we should not allow that effort to roadblock running the current serial tests we have

[jim-minter]

closing for #16919