Repository navigation
Integration test updates - #7196
matthewhughes934 wants to merge 2 commits into
Conversation
rustbot
added
the
S-waiting-on-review
8e4b0a2 to
17eb303
Compare
| // NOTE: existing the behaviour, possibly a bug | ||
| assert_eq!(&stdout, "\n"); |
There was a problem hiding this comment.
Huh, interesting... I feel like this is a bug, I sense that the .trim() you added later is just papering over the newline coming from somewhere.
There was a problem hiding this comment.
Huh, interesting... I feel like this is a bug, I sense that the .trim() you added later is just papering over the newline coming from somewhere.
Yeah, I plan to address this in a followup, just wanted to make this change about restructuring the tests
| remove_file("minimal-config").unwrap(); | ||
| } | ||
|
|
||
| // test writes to file within the repo, rust-lang/rust doesn't like that |
There was a problem hiding this comment.
Could we use a tempdir instead?
There was a problem hiding this comment.
Could we use a tempdir instead?
👍 Another one for my followup
| @@ -113,7 +113,16 @@ fn print_config_errors_minimal_without_input_file() { | |||
| #[test] | |||
| fn print_config_writes_to_file() { | |||
There was a problem hiding this comment.
Presumably we need to also make sure we remove the file if it existed -- otherwise you can get a false green locally from a previous run even if you completely break the logic.
There was a problem hiding this comment.
Presumably we need to also make sure we remove the file if it existed -- otherwise you can get a false green locally from a previous run even if you completely break the logic.
👍 yep, one more for the followup
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
jieyouxu
added
S-waiting-on-review
The main thing here is to be more explicit on assertions: e.g. to answer are we expecting something to be printed to stdout or stderr? I thought about rewriting the existing macro to handle this case too, but couldn't find a good approach for that, and decided that the plain code, while more lines, was more readable.
Firstly, Simplify the directory structure: we only need a single package here, so drag everything up for that. Secondly, make it more explicit exactly which config we expect to read by setting up configs with different options, rather than just ignoring all formatting. Also add tests for other existing behaviour: searching in `HOME` and `XDG_CONFIG_HOME` when no config is given nor found.
a30acc8 to
a8bf62d
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This branch has not been deployed
4 participants
Rewrite
--print-configintegration testsThe main thing here is to be more explicit on assertions: e.g. to answer
are we expecting something to be printed to stdout or stderr?
I thought about rewriting the existing macro to handle this case too,
but couldn't find a good approach for that, and decided that the
plain code, while more lines, was more readable.
Update config discovery integration tests
Firstly, Simplify the directory structure: we only need a single package here,
so drag everything up for that. Secondly, make it more explicit exactly
which config we expect to read by setting up configs with different
options, rather than just ignoring all formatting.
Also add tests for other existing behaviour: searching in
HOMEandXDG_CONFIG_HOMEwhen no config is given nor found.I did not use an LLM to create a change in this PR.
I used an LLM to create a change in this PR, and I have explained below how it was used.