call_python: Rewrite test in Python

[EricCousineau-TRI]

Resolves #7703

Given that this test is failing more frequently in CI, will see if this resolves some of the sticking points.

\cc @jwnimmer-tri

---

This change is 

[EricCousineau-TRI]

+@jamiesnape for feature review, please.

---

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jamiesnape]

I would be surprised if it fixes the flakiness, but certainly an improvement.

---

Reviewed 5 of 6 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

common/proto/BUILD.bazel, line 149 at r1 (raw file):

    ],
    # Do not isolate, as we wish to access neighboring files.
    isolate = False,

We typically use 0 and 1 for booleans in BUILD files. You may wish to shorten the comment to We wish to access neighboring files. since the Do not isolate is self-evident.

---

common/proto/BUILD.bazel, line 152 at r1 (raw file):

    # TODO(eric.cousineau): Find the source of (more) sporadic CI failures, but
    # after refactoring the script into Python.
    # flaky = 1,

I think we keep the flaky until proven otherwise. This is a fairly faithful reproduction of the shell test.

---

common/proto/test/call_python_test.py, line 31 at r1 (raw file):

# `bazel test` can cover the same capabilities as `bazel run` (debugging,
# command-line arguments via command-line, etc).
assert "TEST_TMPDIR" in os.environ, "Must run under `bazel test`"

Not sure I understand the comment, nor its relevance to the code. Aside, you are looking for the --test_arg and --test_output flags (we do not need that in a comment, though).

---

common/proto/test/call_python_test.py, line 34 at r1 (raw file):

# By default, set backend so that the test does not open windows.
os.environ["MPLBACKEND"] = "ps"

As far as I can tell the By default is redundant since I do not see any setting of the environment variable otherwise.

---

common/proto/test/call_python_test.py, line 44 at r1 (raw file):

client_bin = os.path.join(cur_dir, "../call_python_client_cli")
# Change this if you wish to skip plotting testing (useful if debugging).
no_plotting = False

Can we have positive variable name? Also the comment does not add any useful information since the purpose of the variable is self-evident.

---

common/proto/test/call_python_test.py, line 99 at r1 (raw file):

                self.assertIn(server.wait(), server_valid_statuses)
                # Wait until the client has indicated that the server process
                # has run twice.

Why twice?

---

common/proto/test/call_python_test.py, line 101 at r1 (raw file):

                # has run twice.
                wait_for_done_count(2)
                # Kill the client with SIGINT since it's waiting.

Kill the client since it's waiting. would be sufficient given the passing of the SIGINT is pretty obvious.

---

common/proto/test/call_python_test.py, line 104 at r1 (raw file):

                client.send_signal(signal.SIGINT)
            client_status = client.wait()
            if not with_error:

I think if with_error: ... else: ... would be better, those feels like you should be able to contract this to one statement with an int() call.

---

Comments from Reviewable

[EricCousineau-TRI]

+@sammy-tri for platform review, please.

---

Review status: 2 of 4 files reviewed at latest revision, 8 unresolved discussions.

---

common/proto/BUILD.bazel, line 149 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

We typically use 0 and 1 for booleans in BUILD files. You may wish to shorten the comment to We wish to access neighboring files. since the Do not isolate is self-evident.

Done.

---

common/proto/BUILD.bazel, line 152 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

I think we keep the flaky until proven otherwise. This is a fairly faithful reproduction of the shell test.

Done.

---

common/proto/test/call_python_test.py, line 31 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Not sure I understand the comment, nor its relevance to the code. Aside, you are looking for the --test_arg and --test_output flags (we do not need that in a comment, though).

Done.

---

common/proto/test/call_python_test.py, line 34 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

As far as I can tell the By default is redundant since I do not see any setting of the environment variable otherwise.

Done.

---

common/proto/test/call_python_test.py, line 44 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Can we have positive variable name? Also the comment does not add any useful information since the purpose of the variable is self-evident.

Done.

---

common/proto/test/call_python_test.py, line 99 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Why twice?

Done.

---

common/proto/test/call_python_test.py, line 101 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Kill the client since it's waiting. would be sufficient given the passing of the SIGINT is pretty obvious.

Done.

---

common/proto/test/call_python_test.py, line 104 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

I think if with_error: ... else: ... would be better, those feels like you should be able to contract this to one statement with an int() call.

Done.

---

Comments from Reviewable

[EricCousineau-TRI]

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please.
@drake-jenkins-bot mac-sierra-clang-bazel-experimental please.

[jamiesnape]

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[jamiesnape]

Optional builds all passed.

[sammy-tri]

Reviewed 3 of 6 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

common/proto/test/call_python_test.py, line 50 at r2 (raw file):

    values_read = set()
    while done_count < num_expected:
        assert done_count <= num_expected

I'm having trouble figuring out what this assert is guarding inside the while loop.

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

common/proto/test/call_python_test.py, line 50 at r2 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

I'm having trouble figuring out what this assert is guarding inside the while loop.

OK to prevent against a rogue write, or a stale written state. It's already kinda taken care of by the other machinery, so I can remove it.

Would you like me to remove it?

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

common/proto/test/call_python_test.py, line 50 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK to prevent against a rogue write, or a stale written state. It's already kinda taken care of by the other machinery, so I can remove it.

Would you like me to remove it?

Done. Removed.

---

Comments from Reviewable

[sammy-tri]

---

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable