Move requests_unixsocket import outside of UNIXSocketNotebookTestBase.fetch_url()

[hrnciar]

Some tests were failing on Fedora due to missing import of requests_unixsocket. Because of bare exception in wait_until_alive() function, it was difficult to understand where the problem lies.

I would suggest moving the import to the beginning of the test file, so in case of missing module ImportError exception will be raised even before the tests are collected.

Another solution would be to replace exception in the wait_until_alive() function with more specific ones (eg. HTTPError, ConnectionError...) so it will wait only if an exception is related to currently starting notebook and nothing else (like ImportError in this case).

[kevin-bates]

Hi @hrnciar - thank you for submitting this pull request.

The reason for the lazy import is because server extension authors use this module as a library and moving the import to a broader scope was forcing them to unnecessarily install requests_unixsocket - otherwise breaking their CI.

We added requests_unixsocket to the extras_require for 'test' and it seems that your workflow should perform the requisite pip install notebook[test] command prior to running tests - similar to what CI is doing.

[frenzymadness]

That makes perfect sense. What we are trying to accomplish is that a missing dependency in tests is not reported as TimeoutError("The notebook server didn't start up correctly.") from wait_until_alive().

If we cannot move the import to the module level. It might make sense to change the bare Exception to something more specific in this method as previously mentioned.

What do you think?

[kevin-bates]

I see value in that, but rather than potentially break existing reasons to wait, how about checking for ModuleNotFoundError first and let it produce a fatal, but appropriate, message, and let the bare exception act as a fallback. I think that stands the least risk of introducing surprises at this point.

[hrnciar]

@kevin-bates, I have updated PR with ModuleNotFoundError exception.

[kevin-bates]

No need to recreate the error instance, let's just pass it on.

Also, could you please add a comment here, something somewhat generic like the following? This way folks can extend this except clause as needed.

Suggested change

	except ModuleNotFoundError as error:
	raise ModuleNotFoundError(error)
	except ModuleNotFoundError as error:
	# Errors that should be immediately thrown back to caller
	raise error

[kevin-bates]

These changes look good to me - thank you.