Skip to content

Conversation

antecrescent
Copy link

@antecrescent antecrescent commented May 1, 2023

test_info_style_info() and test_info_style_subtitle() fail with Error: Could not find a git repository in '.' or in any of its parents if the current working directory is not a git repository. This breaks workflows of e.g. Linux distributions which download tarballs from Github.

Currently, the fn repo(name: &str) -> Result<Repository> git repository setup function is duplicated across benchmarking, integration and unit tests. This pull requests unifies it under an opt-in library function via the test-utils feature and fixes the aforementioned tests.

I chose to handle this with a feature, because when compiling integration tests, #[cfg(test)] is not active and the normally compiled lib gets linked against them.
Using a workaround, the test-utils feature and all tests depending on it, are enabled by default for testing and benchmarking.

As of now, it is not possible to add dependencies based on optional features, which is why I had to move gix-testtools to [dependencies].

test_info_style_info() and test_info_style_subtitle() fail with
"Error: Could not find a git repository in '.' or in any of its parents"
if the current working directory is not a git repository.

* Provide the git repository setup function as a opt-in library utility.
  Deduplicate the repo setup for benchmarking, integration tests and
  unit tests.
via `cargo fmt`
@o2sh
Copy link
Owner

o2sh commented May 1, 2023

IMO, the issue lies with the style_info and style_subtitle functions. It should not be necessary to create an Info struct just to perform unit testing on these methods. To simplify the unit testing process and avoid using integration testing tools (gix-testtools), we could refactor these functions into standalone helper functions that are independent of the Info implementation.

What do you think @antecrescent ?

smth like:

fn style_info(info: &str, with_color: bool, text_colors: &TextColors) -> String {
    if with_color {
        let info_style = get_style(false, text_colors.info);
        format!("{}", info.style(info_style))
    } else {
        info.into()
    }
}

fn style_subtitle(subtitle: &str, no_bold: bool, text_colors: &TextColors) -> String {
    let subtitle_style = get_style(!no_bold, text_colors.subtitle);
    let colon_style = get_style(!no_bold, text_colors.colon);
    format!(
        "{}{}",
        subtitle.style(subtitle_style),
        ":".style(colon_style)
    )
}

@spenserblack
Copy link
Collaborator

Agree with @o2sh that we're jumping through too many hoops to even set up our tests to be run.

We could also possibly refactor Info to be easier to construct. Right now it's one new method that is, frankly, massive (or at least that's my feeling after using linters that penalize methods with more than 10 lines 😆)

Seems like it's very idiomatic in Rust to have a FooBuilder struct, so maybe that's an option?

// Example:
let info = Info::builder() // returns InfoBuilder::default()
    .no_color_palette()
    .title(Title::builder().build()) // Maybe a title builder since it's a complex struct?
    .build();
// No color palette, no info fields, so now this test can focus solely on the title
assert_eq!(info.to_string(), "Name ...");

Info::new could be refactored to use this builder, and tests could also use the builder.

@o2sh
Copy link
Owner

o2sh commented May 2, 2023

Right now it's one new method that is, frankly, massive (or at least that's my feeling after using linters that penalize methods with more than 10 lines 😆)

I think I've been staring at it for so long that it doesn't even shock me anymore 😄 , but yeah, I agree this constructor needs some serious refactoring.

Seems like it's very idiomatic in Rust to have a FooBuilder struct, so maybe that's an option?

Very cool idea 👍 With this in place, we should be able to add some unit tests on the Info struct.

@antecrescent It's your call if you want to take care of this refactor suggested by @spenserblack . IMO, this may be out of scope considering your initial issue. Just updating style_info and style_subtitle into standalone helper functions should be enough.

@antecrescent
Copy link
Author

Thank you for the thorough reviews. Unfortunately, I am very new to Rust and quite unfamiliar with the code base. I will try to update style_info and style_subtitle until the end of next week.

@o2sh
Copy link
Owner

o2sh commented May 11, 2023

Closed as part of #1047

@o2sh o2sh closed this May 11, 2023
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