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

Tests #10

Merged
merged 28 commits into from Sep 23, 2016

Conversation

Projects
None yet
2 participants
@Apanatshka
Contributor

Apanatshka commented Aug 6, 2016

Writing tests as described in #3.
Going straight for the bonus points: so far I've written quickcheck tests for a number of unit test situations.

TODO list

Unit tests

  • Test the benchmark parser, including the fact that throughput is optional.
  • Test that find overlap finds the correct pairs.
  • Test that find overlap reports correct missing benchmarks (for both old and new).
  • Test that the name determination for the two columns works correctly when given two file paths. (This may require refactoring the names method off of the Args struct.)
  • Test commafy.
  • Test that the name prefix feature works. (Refactor code to get a unit test.)

Integration tests

  • Test some basic example inputs and check that it produces the expected output.
  • Test that passing input on stdin works.
@Apanatshka

This comment has been minimized.

Show comment
Hide comment
@Apanatshka

Apanatshka Aug 20, 2016

Contributor

@BurntSushi: I refactored some code to make it more testable, but didn't do the most minimal change because I saw room for improvement. The changes are in a2caf90.

Contributor

Apanatshka commented Aug 20, 2016

@BurntSushi: I refactored some code to make it more testable, but didn't do the most minimal change because I saw room for improvement. The changes are in a2caf90.

@Apanatshka

This comment has been minimized.

Show comment
Hide comment
@Apanatshka

Apanatshka Aug 21, 2016

Contributor

The last two test can be implemented once uutils/coreutils#959 is resolved, which should provide a nice crate for integration testing of command line tools.

Contributor

Apanatshka commented Aug 21, 2016

The last two test can be implemented once uutils/coreutils#959 is resolved, which should provide a nice crate for integration testing of command line tools.

@Apanatshka

This comment has been minimized.

Show comment
Hide comment
@Apanatshka

Apanatshka Aug 27, 2016

Contributor

Looks like the appveyor build is failing because of rust-lang/rust#35991.

Contributor

Apanatshka commented Aug 27, 2016

Looks like the appveyor build is failing because of rust-lang/rust#35991.

@Apanatshka Apanatshka changed the title from Tests (WIP) to Tests Aug 29, 2016

@Apanatshka

This comment has been minimized.

Show comment
Hide comment
@Apanatshka

Apanatshka Aug 29, 2016

Contributor

@BurntSushi: Ready for review/merge

Contributor

Apanatshka commented Aug 29, 2016

@BurntSushi: Ready for review/merge

@Apanatshka

This comment has been minimized.

Show comment
Hide comment
@Apanatshka

Apanatshka Sep 22, 2016

Contributor

@BurntSushi Still waiting on a review...

Contributor

Apanatshka commented Sep 22, 2016

@BurntSushi Still waiting on a review...

Show outdated Hide outdated Cargo.toml
@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Sep 22, 2016

Owner

@Apanatshka Overall this looks really good. Could you say whether there are any behavior changes here?

Sorry about neglecting your PRs. I've been really into another project, and any leftover time I had after that has gone into wedding planning!

Owner

BurntSushi commented Sep 22, 2016

@Apanatshka Overall this looks really good. Could you say whether there are any behavior changes here?

Sorry about neglecting your PRs. I've been really into another project, and any leftover time I had after that has gone into wedding planning!

@Apanatshka

This comment has been minimized.

Show comment
Hide comment
@Apanatshka

Apanatshka Sep 22, 2016

Contributor

In reverse order:

I've been really into another project,

That doesn't surprise me at all, you seem to be quite the collector :P

and any leftover time I had after that has gone into wedding planning!

If that's not a legitimate "real life" excuse, I don't know what is :) Good luck with that, and remember to enjoy it rather than stressing out ^^

Sorry about neglecting your PRs

I don't mind nagging until you respond if you don't ;)

Could you say whether there are any behavior changes here?

Only one outside observable change that I'm aware of: Newlines in the output are now consistently unix \n. I found during testing that prettytable-rs actually did OS-specific line breaks, but in our own output code that wasn't done. The OS-specific stuff is now optional through a feature flag in that crate, so I turned it off.

Contributor

Apanatshka commented Sep 22, 2016

In reverse order:

I've been really into another project,

That doesn't surprise me at all, you seem to be quite the collector :P

and any leftover time I had after that has gone into wedding planning!

If that's not a legitimate "real life" excuse, I don't know what is :) Good luck with that, and remember to enjoy it rather than stressing out ^^

Sorry about neglecting your PRs

I don't mind nagging until you respond if you don't ;)

Could you say whether there are any behavior changes here?

Only one outside observable change that I'm aware of: Newlines in the output are now consistently unix \n. I found during testing that prettytable-rs actually did OS-specific line breaks, but in our own output code that wasn't done. The OS-specific stuff is now optional through a feature flag in that crate, so I turned it off.

@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Sep 23, 2016

Owner

All right great! Since I merged your other color PR, this one now has a conflict. After a rebase I'll merge. :-)

Owner

BurntSushi commented Sep 23, 2016

All right great! Since I merged your other color PR, this one now has a conflict. After a rebase I'll merge. :-)

Apanatshka added some commits Aug 6, 2016

Turned benchmark name prefix into more testable function
While doing so, removed reading whole files/stdin in favour of
buffered reading.
Final fixes for windows integration tests
Now using windows line endings in expected output when on the windows
platform.
Removed quickcheck property that doesn't hold
Counter-example found in
https://ci.appveyor.com/project/BurntSushi/cargo-benchcmp/build/1.0.23/job/fm2b0kwg993qeuuh
The from_original property should cover the same functionality anyway,
so this doesn't weaken the test suite.
@Apanatshka

This comment has been minimized.

Show comment
Hide comment
@Apanatshka

Apanatshka Sep 23, 2016

Contributor

Hah, the test for the usage string just failed, because the --color option is now added.

Question: Is this even a good test? I basically duplicated the string in the code to a file and test that that's the output on -h/--help...

Contributor

Apanatshka commented Sep 23, 2016

Hah, the test for the usage string just failed, because the --color option is now added.

Question: Is this even a good test? I basically duplicated the string in the code to a file and test that that's the output on -h/--help...

@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Sep 23, 2016

Owner

@Apanatshka Oh, wow, I see. I missed that. I'd throw away that test. It's better to just write new tests for each new flag/option added. I think having to update a separate file every time the usage string changes is probably too annoying.

Owner

BurntSushi commented Sep 23, 2016

@Apanatshka Oh, wow, I see. I missed that. I'd throw away that test. It's better to just write new tests for each new flag/option added. I think having to update a separate file every time the usage string changes is probably too annoying.

Apanatshka added some commits Sep 23, 2016

Remove static included strings from tests
Accidentally left over from old workaround that was supposed to be completely removed in 3cb31d6
@Apanatshka

This comment has been minimized.

Show comment
Hide comment
@Apanatshka

Apanatshka Sep 23, 2016

Contributor

Ok then. The usage string test is removed now.
In a related category, there is still a test in there for the version flag. But it grabs the version from a cargo environment variable, so it shouldn't be fragile.

It's better to just write new tests for each new flag/option added.

Yeah, ideally I'd like to add integration tests for the coloured output.. But the planned "quickly rebase so it can be merged today" is already taking more time that I expected. I should really get back to work now ^^' In 6 hours I'll have time to capture the output with terminal colour codes in a fixture for a test.

Contributor

Apanatshka commented Sep 23, 2016

Ok then. The usage string test is removed now.
In a related category, there is still a test in there for the version flag. But it grabs the version from a cargo environment variable, so it shouldn't be fragile.

It's better to just write new tests for each new flag/option added.

Yeah, ideally I'd like to add integration tests for the coloured output.. But the planned "quickly rebase so it can be merged today" is already taking more time that I expected. I should really get back to work now ^^' In 6 hours I'll have time to capture the output with terminal colour codes in a fixture for a test.

@BurntSushi BurntSushi merged commit b021cab into BurntSushi:master Sep 23, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Sep 23, 2016

Owner

@Apanatshka This is great, thank you!

(Tests for coloring are strictly nice to have. Don't sweat it. :-))

Owner

BurntSushi commented Sep 23, 2016

@Apanatshka This is great, thank you!

(Tests for coloring are strictly nice to have. Don't sweat it. :-))

@Apanatshka Apanatshka referenced this pull request Sep 27, 2016

Closed

write tests #3

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