Include actual and expected values in config test error message #1633
Conversation
rajatjindal
force-pushed
the
debug-config-vars
branch
from
6ac6570
to
89e63c8
Compare
There was a problem hiding this comment.
The behaviour is fine but the naming is confusing in the context of the password fiction. As this is specifically a test case, not something that is meant to motivate understanding, I'd suggest dropping the fiction, and being absolutely clear in the naming and the error message about it being a config value that was not what it should have been.
| ); | ||
|
|
||
| return Ok(http::Response::builder() | ||
| .status(401) |
There was a problem hiding this comment.
I'd leave this as a 500 - it's "something is wrong in our test, something that shouldn't have happened," not an actual auth failure. (Not sure if this means we could go back to using ensure! though.)
There was a problem hiding this comment.
Oops, I see you and @kate-goldenring have already been round and round on this one... sorry for sticking another opinion in! I agree with the previous discussion that it doesn't much matter!
There was a problem hiding this comment.
we can't use ensure! as it panics and returns 500 without useful message. though to remove the fiction part, I would change it to return 500 instead of 401
how I have done in some other tests is to let the lib.rs just return the value and do assertion in test case assertions itself.
@kate-goldenring @itowlson - what do you think about that?
There was a problem hiding this comment.
@rajatjindal That's fine. I don't care vastly about the code. Happy with what you and @kate-goldenring have decided.
There was a problem hiding this comment.
I've changed it back to 500. just because 401 generally means unauthorized and in the context of the test, the user may get confused with the error code.
if we don't agree to 500, a better code would be 400 (bad request).
|
@rajatjindal Also could you please amend the commit message? It makes sense close up, but to someone scanning the history it won't be clear what it refers to. |
itowlson
changed the title
rajatjindal
force-pushed
the
debug-config-vars
branch
from
89e63c8
to
bce21ac
Compare
done. |
| .expect("Should have a password query string"); | ||
| let expected_password = config::get("password")?; | ||
| let expected_config_value = query | ||
| .get("expected_config_value") |
There was a problem hiding this comment.
If you change this, I think you need to change the test that passes the query string? It might be easier to leave the actual query string as was, as you did for the config key. It's only the variables and the message that I found confusing - sorry if I was overemphatic in my suggestions for change.
…ation test Signed-off-by: Rajat Jindal <[email protected]>
rajatjindal
force-pushed
the
debug-config-vars
branch
from
bce21ac
to
c647bf2
Compare
|
ok, sorry for the back and forth on the PR. current state:
|
this allows to debug config vars test by sending expected and actual value back