Open josecelano opened 3 weeks ago
da2ce7
commented
3 days ago Hmmm... I think that having the ERROR go through to the STDERR in tests is a good thing... For these cases we should idivitually look into each on, and see if the error is really an error, or it is limitation of our reporting, or we should lower the warning to WARN... or it is a real error, and we disable the tracing for that individual test, (with a comment why).
mario-nt
commented
3 days ago @da2ce7 I would add a new config option to set the tracing level for the tests in the toml file. And still have the tracing_stderr_init function to override that tracing level on tests when needed, as I think having that function called in every test adds a bit of repeated code.
Regarding having tracing on tests, I think it can be useful for development, but I think it is better if we just show an OK or FAIL result when running them, I will do some research on the topic anyway.
josecelano
commented
1 day ago Hi @da2ce7 and @mario-nt, the reason those tests are returning a 500 error is to keep compatibility with the old API.
Before migrating to Axum, the previous API returned a 500 for all these cases. I decided not to break the contract in the migration and wait until the new API.
These are the cases in tests:
pub async fn assert_token_not_valid(response: Response) {
assert_unhandled_rejection(response, "token not valid").await;
}
pub async fn assert_unauthorized(response: Response) {
assert_unhandled_rejection(response, "unauthorized").await;
}
pub async fn assert_failed_to_remove_torrent_from_whitelist(response: Response) {
assert_unhandled_rejection(response, "failed to remove torrent from whitelist").await;
}
pub async fn assert_failed_to_whitelist_torrent(response: Response) {
assert_unhandled_rejection(response, "failed to whitelist torrent").await;
}
pub async fn assert_failed_to_reload_whitelist(response: Response) {
assert_unhandled_rejection(response, "failed to reload whitelist").await;
}
pub async fn assert_failed_to_generate_key(response: Response) {
assert_unhandled_rejection(response, "failed to generate key").await;
}
pub async fn assert_failed_to_delete_key(response: Response) {
assert_unhandled_rejection(response, "failed to delete key").await;
}
pub async fn assert_failed_to_reload_keys(response: Response) {
assert_unhandled_rejection(response, "failed to reload keys").await;
}
async fn assert_unhandled_rejection(response: Response, reason: &str) {
assert_eq!(response.status(), 500);
assert_eq!(response.headers().get("content-type").unwrap(), "text/plain; charset=utf-8");
let reason_text = format!("Unhandled rejection: Err {{ reason: \"{reason}");
let response_text = response.text().await.unwrap();
assert!(
response_text.contains(&reason_text),
":\n response: `\"{response_text}\"`\n does not contain: `\"{reason_text}\"`."
);
}The tests are all from the API:
servers::api::v1::contract::authentication::should_not_authenticate_requests_when_the_token_is_empty
servers::api::v1::contract::authentication::should_not_authenticate_requests_when_the_token_is_invalid
servers::api::v1::contract::authentication::should_not_authenticate_requests_when_the_token_is_missing
servers::api::v1::contract::context::auth_key::deprecated_generate_key_endpoint::should_fail_when_the_auth_key_cannot_be_generated
servers::api::v1::contract::context::auth_key::deprecated_generate_key_endpoint::should_not_allow_generating_a_new_auth_key_for_unauthenticated_users
servers::api::v1::contract::context::auth_key::should_fail_when_keys_cannot_be_reloaded
servers::api::v1::contract::context::auth_key::should_fail_when_the_auth_key_cannot_be_deleted
servers::api::v1::contract::context::auth_key::should_fail_when_the_auth_key_cannot_be_generated
servers::api::v1::contract::context::auth_key::should_not_allow_deleting_an_auth_key_for_unauthenticated_users
servers::api::v1::contract::context::auth_key::should_not_allow_generating_a_new_auth_key_for_unauthenticated_users
servers::api::v1::contract::context::auth_key::should_not_allow_reloading_keys_for_unauthenticated_users
servers::api::v1::contract::context::stats::should_not_allow_getting_tracker_statistics_for_unauthenticated_users
servers::api::v1::contract::context::torrent::should_not_allow_getting_a_torrent_info_for_unauthenticated_users
servers::api::v1::contract::context::torrent::should_not_allow_getting_torrents_for_unauthenticated_users
servers::api::v1::contract::context::whitelist::should_fail_when_the_torrent_cannot_be_removed_from_the_whitelist
servers::api::v1::contract::context::whitelist::should_fail_when_the_whitelist_cannot_be_reloaded_from_the_database
servers::api::v1::contract::context::whitelist::should_not_allow_removing_a_torrent_from_the_whitelist_for_unauthenticated_users
servers::api::v1::contract::context::whitelist::should_not_allow_whitelisting_a_torrent_for_unauthenticated_users
servers::http::v1::contract::configured_as_whitelisted::and_receiving_an_announce_request::should_fail_if_the_torrent_is_not_in_the_whitelist
servers::http::v1::contract::configured_as_whitelisted::receiving_an_scrape_request::should_return_the_zeroed_file_when_the_requested_file_is_not_whitelistedI think we should do what we agreed on in the meeting. We should:
For example, in this test:
#[tokio::test]
async fn should_not_authenticate_requests_when_the_token_is_empty() {
INIT.call_once(|| {
tracing_stderr_init(LevelFilter::ERROR);
});
let env = Started::new(&configuration::ephemeral().into()).await;
let response = Client::new(env.get_connection_info())
.get_request_with_query("stats", Query::params([QueryParam::new("token", "")].to_vec()))
.await;
assert_token_not_valid(response).await;
env.stop().await;
}We can change the test to something like:
use tracing::Level;
use tracing_subscriber::fmt::TestWriter;
use tracing_subscriber::prelude::*;
#[tokio::test]
async fn should_not_authenticate_requests_when_the_token_is_empty() {
let log_writer = TestWriter::new();
let subscriber = tracing_subscriber::fmt()
.with_writer(log_writer.clone())
.with_max_level(Level::ERROR)
.finish();
let _guard = tracing::subscriber::set_default(subscriber);
let env = Started::new(&configuration::ephemeral().into()).await;
let response = Client::new(env.get_connection_info())
.get_request_with_query("stats", Query::params([QueryParam::new("token", "")].to_vec()))
.await;
assert_token_not_valid(response).await;
env.stop().await;
// Ensure the log contains the expected error
let logs = log_writer.to_string();
assert!(
logs.contains("response failed, classification: Status code: 500 Internal Server Error"),
"Expected error log was not found in the captured logs: {}",
logs
);
}We can start with the most straightforward implementation and see later whether we can extract this into a test helper or move the logic to the configuration::ephemeral() factory. A helper seems like a good option because, in fact, login setup is done for the whole app, not for a specific service (API).
I would fix all tests with this pattern to remove the errors. There are only a few of them.
When you run the tests you see some errors in the stderror
You can run the tests with
command 2>/dev/null, but we should disable that output in tracing when running the tests.