DM-12007 Add tests to display_firefly

[stargaser]

Add a minimal import test to the package. Note: interactive user tests for all backends are in afw/tests/test_display.py

• Added tests/ subdirectory
• Added tests/test_import.py that imports the package, and attempts to connect to a nonexistent host to raise a ws4py.websocket.HandshakeError.
• Added tests/SConscript file as described in the developer's guide.

[timj]

In case you haven't spotted it, don't forget the tests/SConscript file.

[stargaser]

@timj thank you for pointing out about the Sconscript file.

• How do I test that the Sconscript file is having the desired effect? If I just run pytest it auto-discovers the tests without this file.
• I built this package on Jenkins but the test output doesn't list anything about tests in this package. Is there a way to verify the tests are run in CI?
• Would you be willing to review this PR? Or if not, would you please suggest a reviewer?

[timj]

I can review. When you type scons it should now run pytest itself.

[timj]

I fixed the name of the SConscript file so that the tests now get run by scons.

Is there any way to get the test to timeout quicker? The test currently takes 80 seconds for me.

pytest -Wd is reporting a unclosed socket:

ResourceWarning: unclosed <socket.socket fd=14, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('140.252.32.219', 63026)>

[timj]

@stargaser I should have made it more obvious that I had already fixed the problem with the SConscript name and pushed that fix. I see that you overwrote my commit. No problem.

[stargaser]

@timj apologies -- I was trying to squash the commits while leaving yours intact, but it didn't quite work. The original test took 80 seconds to timeout so I made a test that raises an exception faster -- then wanted to clean up. Thank you for reviewing and providing assistance!

[timj]

Ok. Great. I'm a bit concerned by the open socket left lying around. Maybe that's the timeout failing to close it in a finalize block.

[timj]

Did you look at the open socket issue?

[stargaser]

How does the open socket manifest itself?

If the FireflyClient is connected to a valid Firefly server, the HandshakeError is not raised. That would still leave the open socket.

To close the socket, it seems to me that this would work:

    def testConnect(self):
        with self.assertRaises(ws4py.websocket.HandshakeError):
            with lsst.display.firefly.firefly_client.FireflyClient('google.com:80') as fc:
                pass

Since the ticket branch has been merged, but the ticket is still open, can I go back to the ticket branch and add more commits to handle this properly?

[timj]

You can see the problem if you run pytest -Wd. (scons does that itself). I want to make sure that we understand the socket issue. See my earlier comment for the output.

[timj]

You can reuse this branch and merge it again but it might be clearer to do it as tickets/DM-12007-socket and a new PR.

[stargaser]

@timj Another apology: somehow I missed your review completion comment where you pointed out the socket issue and the 80 seconds for timeout.

The changes I made already to decrease the test time also took care of the socket issue. Upon further investigation, I did not have to change the test so drastically. If I create an afw_display.Display with host='echo.websocket.org' and port=8080, the 80-sec timeout and open socket both occur. With `port=80' then both of those problems go away.

Do you want me to add back in the test of creating an afw_display.Display object with invalid host, fixing the long timeout and open socket issues? It would be fast now that I understand what happened.

[timj]

Ok. Good that this is resolved. I think a test with afw_display being used would be good. I'm not sure I understand why switching from port 8080 to port 80 fixes the open socket problem.