[tools] Use opt_in_condition for tests that need commercial solvers

[jwnimmer-tri]

One nice upshot is that Bazel in CI now clearly tells us which tests were skipped by our CI config settings.

Towards #24381. This fixes the spelling of commercial solver testing filtering, but not other kinds of test filtering (e.g., sanitizers). Those still use test tags, and are future work.

Towards #20500.

---

This change is 

[jwnimmer-tri]

Working

Wait for #24386 to land first, to make it easier in case we revert(s).

[jwnimmer-tri]

+a:@tyler-yankee for feature review, please.

@jwnimmer-tri reviewed 10 files and all commit messages, made 4 comments, and resolved 1 discussion.
Reviewable status: LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on tyler-yankee).

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Wait for #24386 to land first, to make it easier in case we revert(s).

Done

---

tools/bazel.rc line 95 at r5 (raw file):

### A configuration that enables all optional dependencies. ###
build:everything --test_tag_filters=-no_everything

FYI The no_everything test tag was dead code, now removed.

---

examples/cubic_polynomial/backward_reachability.cc line 187 at r6 (raw file):

  const solvers::MathematicalProgramResult result = Solve(prog);
  DRAKE_DEMAND(result.get_solver_id() == solvers::MosekSolver::id());

FYI Per the original pull request discussion from 2018 that added this check, it came about because the default solver produced an incorrect plot. However, testing today with MOSEK and non-MOSEK both produce the same plot, so whatever bug it was seems to be fixed.

[tyler-yankee]

I think this approach looks good at a high level. I plan to do a more detailed review pass in a couple days, including sanity checking CI logs, since this PR is somewhat large.

@tyler-yankee reviewed 25 files and all commit messages, and made 4 comments.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on jwnimmer-tri).

---

tools/dynamic_analysis/bazel.rc line 11 at r7 (raw file):

build:kcov --test_tag_filters=-lint,-gurobi,-mosek,-snopt,-no_kcov

### Kcov everything build. ###

BTW I noticed the other day that drake-ci has a code path for --config kcov_everything even though we don't have an "everything coverage" job. This should especially be removed now since Drake will no longer define the config.

---

tools/dynamic_analysis/bazel.rc line 107 at r7 (raw file):

build:memcheck --copt=-O2
build:memcheck --define=USING_MEMCHECK=ON
# We need not check the binary-only GUROBI and MOSEK libraries, since we are

nit typo (ditto below)

Suggestion:

Gurobi

---

tools/dynamic_analysis/bazel.rc line 109 at r7 (raw file):

# We need not check the binary-only GUROBI and MOSEK libraries, since we are
# unable to patch any memcheck complaints discovered in them. (These are already
# off by default in non-everything builds, but setting them redunantly doesn't

nit typo (ditto below)

Suggestion:

redundantly

[jwnimmer-tri]

@jwnimmer-tri reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status: LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on jwnimmer-tri).

[tyler-yankee]

@tyler-yankee reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on jwnimmer-tri).

---

tools/skylark/drake_cc.bzl line 818 at r8 (raw file):

        display = False,
        num_threads = None,
        opt_in_condition = None,

Why are we not using something like drake_cc_optional_test in drake_optional.bzl?

I noticed this because currently //solvers:(gurobi|mosek)_solver_internal_test shows as PASSED in CI while the rest that use Gurobi and MOSEK show SKIPPED, as noted in the PR description. This is because those tests are using drake_cc_optional_googletest with opt_in_condition already. Ideally I think we can unify the two approaches, either by fixing those tests to use this new way, or otherwise creating a new drake_cc_optional_test as I mentioned.

[jwnimmer-tri]

@jwnimmer-tri made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on jwnimmer-tri).

---

tools/skylark/drake_cc.bzl line 818 at r8 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Why are we not using something like drake_cc_optional_test in drake_optional.bzl?

I noticed this because currently //solvers:(gurobi|mosek)_solver_internal_test shows as PASSED in CI while the rest that use Gurobi and MOSEK show SKIPPED, as noted in the PR description. This is because those tests are using drake_cc_optional_googletest with opt_in_condition already. Ideally I think we can unify the two approaches, either by fixing those tests to use this new way, or otherwise creating a new drake_cc_optional_test as I mentioned.

Right. I have a one-line patch I can push that fixes drake_cc_optional_googletest to also report as "SKIPPED".

What I made me realize, though, is that the tests that used to have solver tags are still supposed to be compiled but just not run. (Whereas as drake_cc_optional_googletest is not even supposed to be compiled.) Right now, they are no longer being compiled.

I'll need to ponder that. Working.

[jwnimmer-tri]

@jwnimmer-tri reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on tyler-yankee).

---

tools/skylark/drake_cc.bzl line 818 at r8 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Right. I have a one-line patch I can push that fixes drake_cc_optional_googletest to also report as "SKIPPED".

What I made me realize, though, is that the tests that used to have solver tags are still supposed to be compiled but just not run. (Whereas as drake_cc_optional_googletest is not even supposed to be compiled.) Right now, they are no longer being compiled.

I'll need to ponder that. Working.

Okay, here's the new status quo with e.g. mosek_solver_test and mosek_solver_internal_test, with mosek turned off. The former test doesn't use MOSEK headers so should always be built, even if not run. The latter test can't even compile w/o MOSEK.

Both of their linters are still run with mosek off, but both tests report as skipped. If we introduce a typo into each of their source files, it only shows up as a build error in the first test, showing only that one is compiled by default.

//solvers:mosek_solver_test                                      SKIPPED
//solvers:py/mosek_solver_test_cpplint                  (cached) PASSED in 0.4s
//solvers:mosek_solver_internal_test                             SKIPPED
//solvers:py/mosek_solver_internal_test_cpplint_cpplint (cached) PASSED in 0.6s

[tyler-yankee]

@tyler-yankee partially reviewed 3 files and made 3 comments.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on jwnimmer-tri).

---

tools/skylark/drake_cc.bzl line 818 at r8 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Right. I have a one-line patch I can push that fixes drake_cc_optional_googletest to also report as "SKIPPED".

What I made me realize, though, is that the tests that used to have solver tags are still supposed to be compiled but just not run. (Whereas as drake_cc_optional_googletest is not even supposed to be compiled.) Right now, they are no longer being compiled.

I'll need to ponder that. Working.

Is r10 supposed to also update the aforementioned solvers tests to no longer use drake_cc_optional_googletest?

---

tools/skylark/drake_optional.bzl line 166 at r9 (raw file):

    files to be lint-free, even when the feature is disabled.

    If you simply want to skip running the test under certain contidions, but

nit typo

Suggestion:

conditions

---

tools/skylark/README.md line 71 at r9 (raw file):

Allows a test to be skipped during `bazel test //...` based on a specific
condition. When used with drake_cc_googletest or drake_cc_googletest, the test

nit typo

Suggestion:

drake_cc_test or drake_cc_googletest

[tyler-yankee]

@tyler-yankee reviewed all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on jwnimmer-tri).

---

tools/skylark/drake_cc.bzl line 818 at r8 (raw file):

Previously, tyler-yankee (Tyler Yankee) wrote…

Is r10 supposed to also update the aforementioned solvers tests to no longer use drake_cc_optional_googletest?

ha, looks like we published at the same time ...

That WFM. thanks!

[jwnimmer-tri]

@jwnimmer-tri reviewed 2 files and all commit messages, and resolved 2 discussions.
Reviewable status: LGTM missing from assignee tyler-yankee, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on jwnimmer-tri).

[jwnimmer-tri]

+a:@rpoyner-tri for platform review per schedule, please.

[tyler-yankee]

@tyler-yankee reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform) (waiting on rpoyner-tri).

[jwnimmer-tri]

@drake-jenkins-bot mac-arm-sequoia-clang-bazel-experimental-release please

[jwnimmer-tri]

@drake-jenkins-bot mac-arm-sequoia-clang-bazel-experimental-release please

[rpoyner-tri]

@rpoyner-tri reviewed 26 files and all commit messages, and made 1 comment.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),tyler-yankee (waiting on jwnimmer-tri).