Remove some "Env::close" noise from unit test output

[scottschurr]

Env::close() calls fail without admin privileges

Context of Change

The unit tests have been echoing something about Env.close() for a long time. But it was not obvious why that was or if anything should be done about it. I investigated and found a couple of problems:

1. There were a few unit tests calling Env::close() when the test was set up without admin privileges.
2. The complaint coming out of Env echoed resp["result"]["status"] which was NULL. So the user was not informed about the reason for the complaint.

This change fixes both of those problems:

1. It modifies the unit tests that were calling Env::close() without admin privileges. They no longer call Env::close().
2. It modifies the log write from Env to include the reason that the Env::close() call failed.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• Tests (fixes pre-existing tests for better behavior)

Unless we're documenting the output of the unit tests, I don't think there's any reason to include this change in the release notes.

This change only affects unit test code. Other than running the unit tests, no further testing should be required.

[ghost]

My observation was that failing Env::close() is the reason to random fail unit tests on Mac during CI testing. Does this PR fix this issue?

[scottschurr]

@cdy20, no, unfortunately this change does not fix the Mac unit test failures during CI. The Env:close() is just log output. The unit test does not consider the output to correspond to a failure.

[ximinez]

Looks great. I did an experiment changing the tests back to the old behavior, and verified that the HTTP error looked much better in the log. Then with the full changes, no error.

[scottschurr]

Rebased to 1.8..0-b2. @cdy20, will you have time to weigh in on this pull request? If you're too busy I can look for a different reviewer.

[ghost]

Generally, it is not a good practice to review code of more experienced author by less experienced reviewer. For example, I don't understand why we can just remove env.close() in several places, but I think that the author knows it better.

[scottschurr]

@cdy20, oh, but it's fair to ask questions. The Env::close() calls that were removed occurred in places in the unit tests where the client does not have admin permission. Without admin permission the Env::close() calls did not advance the ledger (as was intended), but they did cause annoying error messages during the unit tests. The best solution was simply to remove the offending Env::close() calls.

A few of the Env::close() calls were conditionalized on admin. So if that test is running with admin permissions the Env::close() call is performed and does what is intended. Otherwise the call is not performed, which removed the annoying error message.

I hope that helps.

[ghost]

I hope that helps.

I understood. My view is that one should check that the value returning by env.close() is false in these places instead of removing it. As for messages, may be one can introduce silentClose() method which will not print messages.

In any case, my approval sign is grey and I can't make it green.)

[ximinez]

My view is that one should check that the value returning by env.close() is false in these places instead of removing it. As for messages, may be one can introduce silentClose() method which will not print messages.

Interesting idea. How hard would it be to detect if the env had admin access in close()? We could potentially suppress the message there.

[scottschurr]

@ximinez, it is interesting, but I'm not keen on leaving an env.close() in the source of a test where that close() doesn't actually happen. Seems misleading to me. So I'm happier just removing from the source code the calls to close() that have no effect. From that perspective the error message coming out of close() seems like a good idea.

I could potentially be persuaded otherwise.