Fix test_logging_to_driver and test_not_logging_to_driver

[raulchen]

Why are these changes needed?

These 2 tests are flaky on CI. Because sometimes the previous autoscaler tests will start background threads and print the following errors to stderr.

Traceback (most recent call last):
  File "/home/travis/miniconda/lib/python2.7/threading.py", line 801, in __bootstrap_inner
    self.run()
  File "/home/travis/build/ray-project/ray/python/ray/autoscaler/updater.py", line 151, in run
    raise e
AssertionError: Unable to SSH to node

The purpose of these tests should be to verify that the logs are redirected (or not redirected) to driver stdout. So there's no need to check stderr. However, we should also fix the issue of not stopping autoscaler background threads in a different PR.

What do these changes do?

Related issue number

Linter

• I've run scripts/format.sh to lint the changes in this PR.

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/16326/
Test FAILed.

[robertnishihara]

@raulchen @ericl how hard would it be to make sure the autoscaler test shuts down properly? That seems like the right fix.

[robertnishihara]

This makes me realize that this line should have been

     assert len(error_lines) == 0, error_lines

so that we can what the stderr was in the case of error

[robertnishihara]

If we remove this check, then we should include a comment that explains what goes wrong if we do check it. Since people (myself included) will be very tempted to bring back this check.

[robertnishihara]

@raulchen @ericl how hard would it be to make sure the autoscaler test shuts down properly? That seems like the right fix.

[robertnishihara]

Unrelated to the test failure, but in this test we should really be checking that we don't get any unintended log messages (or duplicates). Especially since @stephanie-wang saw some duplicates recently.

[raulchen]

@raulchen @ericl how hard would it be to make sure the autoscaler test shuts down properly? That seems like the right fix.

I agree that autoscaler issue should be fixed. But I'm not familiar with autoscaler and don't know how to fix that.
Looking at this test_logging_to_driver test, I think its purpose is to verify that the worker logs will be sent to driver stdout. So I think this test doesn't need to care about stderr. And a more accurate way to test this is to mock the print_logs functions, instead of just checking stdout output.
But for now, I think this PR is enough for unblocking the CI first.

[robertnishihara]

@raulchen this looks good to me. I pushed an additional comment. Does that look good to you?

[raulchen]

@robertnishihara thanks. looks good

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/16363/
Test PASSed.