Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(git_commit): fix potential test failure #4734

Conversation

Foster0123
Copy link
Contributor

Starship git_commit latest tag lightweight test case failed, i fixed the error to pass the test case

@@ -437,7 +437,7 @@ mod tests {
.output()?;

create_command("git")?
.args(["tag", "v2"])
.args(["describe" , "tag" , "v2"])
Copy link
Member

Choose a reason for hiding this comment

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

What sort of failure are you seeing without this? The code here is meant to create a tag, not to run describe. The git tests can have intermittent failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i ran the cargo tests, and saw a test case failed, after adding the describe tag it passes the test case

Copy link
Member

@davidkna davidkna Dec 18, 2022

Choose a reason for hiding this comment

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

But does it pass without your change if you run it again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error
before making changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix
after making changes

Copy link
Member

Choose a reason for hiding this comment

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

Does it work if you replace:

 let git_tag = create_command("git")?
            .args([
                "for-each-ref",
                "--contains",
                "HEAD",
                "--sort=-taggerdate",
                "--count=1",
                "--format",
                "%(refname:short)",
                "refs/tags",
            ])

with this?

 let git_tag = create_command("git")?
            .args(["describe", "--tags"])

If not, please close this and open a bug report, because in my opinion it would mean that the behavior and not the test is incorrect somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it worked after i replaced the code as u mentioned

Copy link
Member

Choose a reason for hiding this comment

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

In that case, please change your PR to use that instead. I would also appreciate if you changed the other tag tests to use git describe --tags as the baseline instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if u mean changing the block of code u mentioned , then yes, i changed it. there was another instance of same code, so changed that as well with git describe --tags. please review the changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ran the test cases, and all of those passed without failures

@davidkna davidkna changed the title Git commit test case fixed fix(git_commit): fix potential test failure Dec 18, 2022
src/modules/git_commit.rs Outdated Show resolved Hide resolved
Co-authored-by: Thomas O'Donnell <andytom@users.noreply.github.com>
Copy link
Member

@davidkna davidkna left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

4 participants