nikola serve: Better error handling for already used port. Fixes #3824

[arunpersaud]

Pull Request Checklist

• I’ve read the guidelines for contributing.
• I updated AUTHORS.txt and CHANGES.txt (if the change is non-trivial) and documentation (if applicable).
• I tested my changes.

Description

Adds a quick test for the exception raised and prints out better error message

[arunpersaud]

I fixed using the logger and the f-string issue, but I'm not sure how to test this in pytest... e.g. how do I run 2 servers on the same port and how to I run them on different ports and what do I assert? I added 2 tests, but they are currently not working. Any help would be appreciated.

[aknrdureegaesr]

I wouldn't run two servers. I would simply open a port and then run the server on the same port.

For how to open a port, check file dev_server_test_helper.py, in particular the method find_unused_port(). It opens a port (or a socket, to be nit-picky about this), leaving the choice of port to the operating system. This will cause the OS to pick a port that's free to be opened. Then my code closes the port right away and returns the port number. As that port was free to be opened before, and the code closed the socket right away, it will be free to open after the find_unused_port() code ran. (Also, as far as I know, the OS will not hand that same port number to any other application asking for a free port number any time soon. I'm not sure that behavior is officially documented.)

You could open a port, but leave it open, and, while it is still open, ask the dev server to service that port. This should not work. This should trigger the error condition you are trying to test.

Finally, after the test is through (either showing the error text was what we expect it to be, or else the test fails), close the port again.

[aknrdureegaesr]

Does that idea of mine make sense to you, or do you want more detailed help on that? It is not my intention to demand big tests of you for a simple change, and then leave you "out alone in the rain", so to speak.

I'd be willing to come up with a test myself, if you want me to (and have a few day's time).

[aknrdureegaesr]

I had a bit of time on my hand and was motivated, so I've been coming up with a test for your code. I hope you feel that's helpful and not interference.

That test now sits in a pull request https://github.com/arunpersaud/nikola/pull/1/files which targets your branch, the branch that's again below this pull request. So if you think that's a good test, you can accept that pull request, and the test will show up as part of this pull request.

At the end of the day, we hope this will be merged here. If so, it will be all one commit, which will appear to be by you, somewhat eclipsing the fact that part of the work was by me. I don't much care, that's fine with me.

[arunpersaud]

@aknrdureegaesr Thanks again for the help with the test!

[aknrdureegaesr]

Sorry, my fault: We are doing pioneer work, the first test that actually checks on our exception handling.

As often, Windows behaves a bit differently, so we get a different exception than the one we are testing for in this PR. Thus the changed error handling from my PR #3829 now gets used. But that change needs an attribute site.show_tracebacks. In production, that is not a problem. But the FakeSite we use in the tests does not provide that yet.

So this should also be set in helper.py in the FakeSite constructor, preferably one line below self.debug = True we need self.show_tracebacks = True. Strictly speaking, I should open another PR for just that. Would you add it to this one, please?

When that has been done, the error for Windows will fully emerge. Other than Linux, as far as I understand, Windows differentiates between a socket opened by the same program and a socket opened by another program. We test the "port already open" error condition here via opening a socket by the same (test) program. So this test does not carry over to Windows gracefully.

There is no Windows machine in this household. So I don't volunteer developing a test for Windows. If we have no such volunteer, I suggest to simply not do the assert re.match thingy if running under Windows. We should probably see the original Windows exception, instead of the result being 3; maybe set show_tracebacks to False in MyFakeSite to at least see result being 3.

Gory detail: I'd try platform.system and check for == "Windows".

We then do not know whether this PR works under Windows as well. It might, but I'm not sure. I'm confident it won't hurt.

[arunpersaud]

I also don't have a windows computer here, so I cannot test this. I added the two items you mentioned. Let me know if this is what you had in mind.

[aknrdureegaesr]

Imagine someone like you stumbles over this in a year or two. This line raises an eyebrow and leaves the puzzling question to be answered: "Why?"

Can you please add a comment here that'll help that someone?

(We need our Nikola community members and want to make life nice for them!)

[aknrdureegaesr]

We now see the original Windows exception. We would like to see the result being 3. I think we'll get that by setting show_tracebacks to False in MyFakeSite.

It should be True in general, which is what FakeSite now has, but we need False for this particular test, with MyFakeSite being the convenient place to get that.

[arunpersaud]

ok, so I would set site.show_tracebacks=False inside the test (say right at the top of the test). Is this only needed for windows or for all tests?

I can add some comments later today and can also test this on linux and mac.

[arunpersaud]

Next version ready for review.

[arunpersaud]

Just checking in on the status of this.