[test] the static-file-server example #3297
Conversation
szabgab
force-pushed
the
test/static-file-server
branch
2 times, most recently
from
b058ad4 to
0311d03
Compare
| const JS: &str = "text/javascript"; | ||
| const HTML: &str = "text/html"; | ||
|
|
||
| async fn get_page(app: Router, path: &str) -> (StatusCode, String, String) { |
There was a problem hiding this comment.
Use headers::ContentType instead of String.
And I'd argue returning Bytes or Vec<u8> for content is also better than forcing this to be String though I could see an argument for making sure that the returned content is valid UTF-8 before also checking that the contents match though I wouldn't subscribe to that. But feel free to ignore this.
| let status = response.status(); | ||
| let content_type = match response.headers().get("content-type") { | ||
| Some(content_type) => content_type.to_str().unwrap().to_owned(), | ||
| None => String::new(), |
There was a problem hiding this comment.
Empty header value is not the same thing as a missing header so I'd generally not recommend anyone to do this. returning Option would be more correct.
You even later in this file check for "" which probably is trying to check for the missing header.
| @@ -0,0 +1,64 @@ | |||
| use super::*; | |||
| use axum::http::StatusCode as SC; | |||
There was a problem hiding this comment.
This is really just an opinion, but I don't like this rename. StatusCode is not that long of a name, it's a bit clearer to people what this is when they read the code without also reading imports and you even use both StatusCode and SC in this file. Which happens to work just because you also have use super::*.
This might be even more contentious opinion especially in tests, but I'd be in favor of not using * in imports, even here.
| const INDEX_HTML: &str = include_str!("../assets/index.html"); | ||
| const SCRIPT_JS: &str = include_str!("../assets/script.js"); | ||
|
|
||
| const JS: &str = "text/javascript"; | ||
| const HTML: &str = "text/html"; |
There was a problem hiding this comment.
Personally, I'd prefer more descriptive names like INDEX_HTML_CONTENT and CONTENT_TYPE_JAVASCRIPT. It's much clearer what you're doing especially in calls like check(app(), "/assets/", SC::OK, HTML, INDEX_HTML).await;.
Also use ContentType as the type instead of &str for the latter two. I don't think it can be const then but static really is good enough.
There was a problem hiding this comment.
Thanks for the feedback.
Right, so I shortened all the names to let fmt put them (most of them) on one line, but I guess it is better to have longer names even if that means the tests are then in multiple lines.
Or maybe you have an idea how to arrange things to have both?
There was a problem hiding this comment.
I don't see how to have both, but more lines with more descriptive names are to me better than the alternative.
szabgab
force-pushed
the
test/static-file-server
branch
from
8ccd273 to
aff31c8
Compare
This time I put the tests in a separate file to reduce the cognitive load for someone learning axum.
See #3263