Fix Staticfiles 404.html in HTML mode

[WhiteApfel]

In StaticFiles html-mode, when 404.html and index.html files were missing, a FileNotFoundError exception was rised. As I understand it, a non-existent file was checked in the os.stat function

However, this is not the whole problem. Even if 404.html exists, it might not be returned, because after checking for the existence of index.html, a basic 404 exception is returned, rather than returning a 404.html file with the correct status code

Therefore, I moved the check for the existence of a 404.html to the end, when index cannot be found. I also considered it correct to move the file absence exception to the lookup_path and, in case of absence, return an empty string and None, so as not to process Exception several times.

Sorry for my bad english

[WhiteApfel]

I looked at the history of file changes. At 1db8b5b (#1220) has undergone "devastating" changes. For some reason, check of the 404.html was moved up there. I read the story (#1123, #1124, #1220), but didn't find an explanation for this action

Obviously, we must first check {path}/index.html, and only then check 404.html

[aminalaee]

I think that was moved by mistake.
Maybe you can add tests for it so this won't happen again?

[WhiteApfel]

Hmmm... @aminalaee, I don't understand the testing system well, but I have a problem

Tests fail due to HTTPException being called:

>       raise HTTPException(status_code=404)
E       starlette.exceptions.HTTPException

../starlette/staticfiles.py:150: HTTPException

I've seen this same construct in other tests. And no exception is raised there. I need help with this.

Even while writing the tests, I noticed that now when 404.html or index.html is not found, the '/' at the end of the URL is removed.
I just took the liberty of removing these lines from my tests (b564f1f), because they are not target, however, they do not exclude the presence of "anomalous" behavior

[aminalaee]

@WhiteApfel

If I understand correctly, you need to write a test to catch an exception?
I think we're using py-test for that, something like this:

with pytest.raises(RuntimeError) as exc_info:
        client.get("/example.txt")

This ensures calling the GET will raise the specified exception.

As for the removal of the tests, I'd rather we don't change them, that would lead to un-intented changes.

[WhiteApfel]

@aminalaee

As for the removal of the tests, I'd rather we don't change them, that would lead to un-intented changes.

#1314 (comment)

response = client.get("/dir/")
assert response.url == "http://testserver/dir/"

response = client.get("/dir")
assert response.url == "http://testserver/dir/"

Oh, I'm sorry. I made a typo during preliminary testing. Corrected. In some cases a '/' should appear and in others it should not. I got the opposite results due to a typo, but now everything is fine ✨

[WhiteApfel]

I think that's all. I don't plan to make any more changes

[aminalaee]

Thank you for this.