Update test case for VideoRecorder

[XuehaiPan]

Refer to #2318 for more details.

[jkterry1]

@XuehaiPan this fixes both of those notes right?

[XuehaiPan]

This PR takes the tests for closer back for the first comment. #2318 (comment)

As commented in #2318 (comment) (the second comment) and #2318 (comment), we could remove all closers in gym entirely (not only for VideoRecoder, but also for Monitor, etc.), and call close() in the __del__() method.

[vwxyzjn]

I like the idea of having "close() in the __del__() method", which seems like a much cleaner solution than a customized Closer. If we move forward with this idea, would it make the test cases cleaner?

[XuehaiPan]

I like the idea of having "close() in the __del__() method", which seems like a much cleaner solution than a customized Closer. If we move forward with this idea, would it make the test cases cleaner?

Definitely. This will make both the implementation and the tests simpler for classes with a close() method (e.g., Env, Monitor, VideoRecorder, etc.). But we should be careful if we add this behavior to the fundamental base classes (Env, Wrapper, etc.).

First, If we want to test the side effect (e.g. create a video file) for a close() method that called implicitly by __del__() (the test_autoclose in this PR), we should do explicit garbage collection and wait for a few seconds (as we cannot know when will the Python interpreter do a garbage cleaning):

import time
import gc

rec_path, proc, num_registered = record()

gc.collect()  # do explicit garbage collection for test
time.sleep(5)  # wait for subprocess exiting

assert proc.poll() is not None  # subprocess is terminated

Second, for some classes, we should and an attribute _closed to instances to avoid duplicate closeing.

def duplicate_close():
    env = MyEnv()
    env.close()  # explicit closing

    # After the function exiting
    # `__del__()` will call `close()` again

duplicate_close()

[jkterry1]

@XuehaiPan can you please make the remaining requested changes (by costa and the second comment) so this can be merged?

[XuehaiPan]

I think the first commit (a888dfa) is enough to reply to both of the comments. I have removed closer for VideoRecorder in the new commit (78e1edb) to simplify the tests. (#2404 (comment) and #2404 (comment))