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

Refactor the binary tests #2344

Merged
merged 3 commits into from May 29, 2020

Conversation

Urhengulas
Copy link
Contributor

@Urhengulas Urhengulas commented May 28, 2020

This PR applies various refactoring to tests/binary.rs:

  • add
    fn get_common_cmd(outfile: &str) -> Command
  • combine related arguments in Command.args(&[arg1, arg2])
  • refactor fn get_tempfile_path(extension: &str) to return OsString
  • don't store Command in intermediate variable

Fixes #2337

tests/binary.rs Outdated Show resolved Hide resolved
tests/binary.rs Outdated Show resolved Hide resolved
@coveralls
Copy link
Collaborator

coveralls commented May 28, 2020

Coverage Status

Coverage remained the same at 82.195% when pulling a52ed12 on Urhengulas:2337-refactor-the-binary-tests into 5ee1e71 on xiph:master.

Copy link
Collaborator

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

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

Thank you for your pull request, I already commented few functional issues, here the annoying non-functional nits :)

  • Faktor -> Refactor, Factor in/Factor out
  • Do not put the issue number in the PR title, it does not autolink, put it in the body or, even better, put in the commit message body as Fixes #2337.

Thank you a lot :)

@lu-zero lu-zero changed the title 2337 refactor the binary tests Refactor the binary tests May 28, 2020
@Urhengulas
Copy link
Contributor Author

Thanks for the review 👍

functional

I added my reasoning to your comments.

non-functional

I normally add the Fixes #... in the commit message, if the fix is only 1 commit. If the fix consists of multiple commits I am always not sure in which commit to put it. 🙈

But, since I have the Fixes #... in the body of the PR it should be fine, right?

@Urhengulas Urhengulas force-pushed the 2337-refactor-the-binary-tests branch from 93568d2 to c977c35 Compare May 28, 2020 12:05
@Urhengulas Urhengulas requested a review from lu-zero May 28, 2020 12:42
@lu-zero
Copy link
Collaborator

lu-zero commented May 28, 2020

Thanks for the review 👍

functional

I added my reasoning to your comments.

non-functional

I normally add the Fixes #... in the commit message, if the fix is only 1 commit. If the fix consists of multiple commits I am always not sure in which commit to put it. 🙈

But, since I have the Fixes #... in the body of the PR it should be fine, right?

It is fine.

tests/binary.rs Outdated
.arg(&passfile)
get_common_cmd(outfile)
.args(&["--reservoir-frame-delay", "14"])
.args(&[OsStr::new("--first-pass"), passfile])
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is more readable to use arg().arg() 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.

That's true.

@Urhengulas Urhengulas force-pushed the 2337-refactor-the-binary-tests branch from e984ddb to 5e0d3ff Compare May 28, 2020 16:56
@Urhengulas
Copy link
Contributor Author

@lu-zero I dropped the two commits. Should be fine now.

@Urhengulas Urhengulas requested a review from lu-zero May 28, 2020 16:59
Copy link
Collaborator

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

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

Looks good, please rebase the set.

@Urhengulas Urhengulas force-pushed the 2337-refactor-the-binary-tests branch from 5e0d3ff to a52ed12 Compare May 28, 2020 22:27
@lu-zero lu-zero merged commit 4ec7e54 into xiph:master May 29, 2020
@Urhengulas Urhengulas deleted the 2337-refactor-the-binary-tests branch June 9, 2020 09:43
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.

Refactor the binary tests
3 participants