Skip to content

Conversation

@atluft
Copy link
Contributor

@atluft atluft commented Oct 12, 2022

Another PR to explore testing using the git-testtool module.
This test is intentionally trying to open a "bare git" repo that onefetch should flag as an error and provide a specific error message.

All comments are welcome!

@atluft
Copy link
Contributor Author

atluft commented Oct 13, 2022

TLDR: to test the expected repo, use a mut on config variable and change config.input after initialization.

The discover function was walking back up the tree and finding the onefetch repo itself instead of the intended bare repo created for the test. In most cases that is what any user would want, but in this case the test is to try to open a bare repository and observe that the error is triggered.

Copy link
Collaborator

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usage of git_testtools generally looks fine, and I chose not to comment on the test itself is conducted as it might be what's desired in this codebase.

src/info/mod.rs Outdated
#[test]
fn test_bare_repo() -> Result<()> {
let repo = test_repo(&"bare_repo.sh".to_string())?;
let mut config = Config::parse_from(&["."]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"." is definitely picking up the onefetch repository because the CWD is the onefetch repository, not the test directory. Passing repo_path from line 373 might be better, but I acknowledge that I don't understand what parse_from(…) really does.

Here the actual repo seems entirely unused.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atluft You might want to look at some of Rust's macros, and Cargo's default environment variables.
For example, env!("CARGO_MANIFEST_DIR") should get you the path to the root of this crate, which you can then concatenate.

Copy link
Contributor Author

@atluft atluft Oct 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I need help. Coaching appreciated on this one.

I was trying to cover this specific code:

.context("please run onefetch inside of a non-bare git repository")?

I used repo to create the git bare repo for testing.
The code here is the only way I could figure out how to assign config.input to the desired repo's test directory instead of having it investigate . which is the onefetch repo and is never bare.

I don't understand how to set config.input to anything but .

TRY 1

let mut config : Config;
config.input = repo.path().to_path_buf();

That was rejected like this:

385 |         config.input = repo.path().to_path_buf();
    |         ^^^^^^^^^^^^ `config` partially assigned here but it isn't fully initialized

TRY 2 and 3

let config = Config::parse_from(&[repo.path().to_path_buf()]);
let config = Config::parse_from(&[repo.path().to_display().to_string()]);

parse_from seems to come from here, but I don't know how to read this code, notice the default value is .

#[derive(Clone, Debug, Parser, PartialEq, Eq)]
#[command(version, about, long_about = None, rename_all = "kebab-case")]
pub struct Config {
    /// Run as if onefetch was started in <input> instead of the current working directory
    #[arg(default_value = ".", hide_default_value = true, value_hint = ValueHint::DirPath)]
    pub input: PathBuf,

Copy link
Collaborator

@spenserblack spenserblack Oct 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atluft I'll try and dig into this more to help later, but I hope this can get you started:

parse_from comes from the Parser, and is documented here. There's definitely a lot of macro usage, which can make things less explicit, but what we're saying is

  • input is a command-line argument
  • if a value wasn't passed to the command-line, input is "."
  • input's default value is not shown in the help message (onefetch -h)
  • input should look like a directory (useful for completion scripts)

It looks like you've already figured this out, but you should be able to just pass an iterator of the command-line arguments into parse_from.

Also, if you're going to build the Config struct manually, you will need to define all fields, not just input.


Keep in mind that testing is only easy when the project is testable. The design had progressed a lot before testing was considered, so some refactoring may need to be done to make the code more testable.
For instance, for your 1st try, it would be a lot easier if Config implemented Default. Which is kind of implemented here:

onefetch/src/cli.rs

Lines 259 to 288 in 00d3905

fn get_default_config() -> Config {
Config {
input: PathBuf::from("."),
ascii_input: Default::default(),
ascii_language: Default::default(),
ascii_colors: Default::default(),
disabled_fields: Default::default(),
image: Default::default(),
image_protocol: Default::default(),
color_resolution: 16,
no_bold: Default::default(),
no_merges: Default::default(),
no_color_palette: Default::default(),
number_of_authors: 3,
exclude: Default::default(),
no_bots: Default::default(),
languages: Default::default(),
package_managers: Default::default(),
output: Default::default(),
true_color: When::Auto,
show_logo: When::Always,
text_colors: Default::default(),
iso_time: Default::default(),
email: Default::default(),
include_hidden: Default::default(),
r#type: vec![LanguageType::Programming, LanguageType::Markup],
completion: Default::default(),
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @spenserblack,
Very helpful, especially the link to parse_from
Will try to create a refactoring for testing idea, for review here.

I agree this current approach isn't great, thank you for the ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants