ContentsHandler return 404 rather than raise exc - Githubissues

jupyter-server / jupyter_server

The backend—i.e. core services, APIs, and REST endpoints—to Jupyter web applications.

https://jupyter-server.readthedocs.io

BSD 3-Clause "New" or "Revised" License

489 stars 308 forks source link

ContentsHandler return 404 rather than raise exc #1357

Closed bloomsa closed 1 year ago

bloomsa commented 1 year ago

this addresses #1328. This works based on my testing but I'm more than happy to make structural changes to clean the code up. I'm admittedly not very well versed in tornado. Thanks in advance for feedback :)

bloomsa commented 1 year ago

I see one of the test actions, build (windows latest 3.9), failed but I'm not sure it's related since the code change doesn't seem to touch the affected test case

blink1073 commented 1 year ago

I see one of the test actions, build (windows latest 3.9), failed but I'm not sure it's related since the code change doesn't seem to touch the affected test case

That is a known flaky test.

blink1073 commented 1 year ago

Changes look good! Can you please add a test in https://github.com/jupyter-server/jupyter_server/blob/main/tests/services/contents/test_manager.py that tries to get a file that doesn't exist and ensures that a 404 is raised?

bloomsa commented 1 year ago

Changes look good! Can you please add a test in https://github.com/jupyter-server/jupyter_server/blob/main/tests/services/contents/test_manager.py that tries to get a file that doesn't exist and ensures that a 404 is raised?

done in my latest commit. couple of questions though

do you know what the "w" argument is on cm.get here? I'm kind of confused about it as "w" doesn't really make sense for the content argument typed as a bool (ref) is it maybe legacy from a previous iteration of the get method?
I tweaked the structure of the assertion a bit compared to the previous two assertions in the test case because I couldn't get the with pytest.raises(HTTPError) as excinfo: to be satisfied. I don't really understand how the assertions above it pass okay - my understanding is that the except should catch the exception and not trickle it up to the with. Do you want me to reformat those other assertions too? (or am I doing something totally wrong here 😅 )

blink1073 commented 1 year ago

Hey @bloomsa, I agree the w is a logic error currently. I also think the previous blocks probably suffered from the move from nose to pytest are incorrect as well. The previous one isn't actually testing anything. 😅

bloomsa commented 1 year ago

I'm working on fixing the test cases for windows (actually led me to a bug in the tests that I'll fix 👍 ) would you like me to fix these as part of this PR? I can do it no problem @blink1073

I agree the w is a logic error currently. I also think the previous blocks probably suffered from the move from nose to pytest are incorrect as well.

blink1073 commented 1 year ago

Yes please!

blink1073 commented 1 year ago

@Wh1isper, I'll wait for you to take another look.

Wh1isper commented 1 year ago

@blink1073 LGTM, thx