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

test cleanup: Remove Cargo's internal hamcrest module #5742

Closed
alexcrichton opened this issue Jul 18, 2018 · 4 comments · Fixed by #5945
Closed

test cleanup: Remove Cargo's internal hamcrest module #5742

alexcrichton opened this issue Jul 18, 2018 · 4 comments · Fixed by #5945
Labels
A-testing-cargo-itself Area: cargo's tests

Comments

@alexcrichton
Copy link
Member

This used to be an upstream crate and we switched due to it being outdated, and otherwise nowadays it's very much showing its age! We primarily use this module to assert various properties of each executed command, but nowadays we should stop using hamcrest in favor of a new form of specifying what should happen.

I'm personally partial to the system I made for cargo-fix the other day for a replacement which features:

  • A builder style instead of a method-oriented style of building up a command
  • By default asserts output status is 0 (unless otherwise specified)
  • Lifts the awesome idea of supporting foo("arg1 arg2 arg3") instead of using arg("arg1").arg("arg2")...
  • Has a vastly simplified method of matching output (avoids things like [/] and dealing with things like cwd changing)
  • Was relatively simple to implement!

I think it'd be worth moving such a system into Cargo and migrating tests to use this assertion builder as well, I think it'd make assertions more concise and be a strong step forward to removing the hamcrest module.

@dwijnand
Copy link
Member

💯 for removing the confusing [/] shenanigans!

Thus I've an interest in this issue, but I'll have to see if I'm able to do it.

@dwijnand
Copy link
Member

dwijnand commented Aug 2, 2018

Lifts the awesome idea of supporting foo("arg1 arg2 arg3") instead of using arg("arg1").arg("arg2")...

Looks like @matklad took care of that a little while ago! 🎉

#5197

bors added a commit that referenced this issue Aug 2, 2018
bors added a commit that referenced this issue Aug 2, 2018
Cleanup tests by using single string commands, split on whitespaces

Refs #5742
bors added a commit that referenced this issue Aug 3, 2018
Default test support's Execs to exit code 0

Refs #5742
bors added a commit that referenced this issue Aug 18, 2018
Collapse ProcessBuilder::arg calls in tests

Refs #5742
More of #5854
bors added a commit that referenced this issue Aug 19, 2018
Collapse ProcessBuilder::arg calls in tests

Refs #5742
More of #5854
bors added a commit that referenced this issue Aug 28, 2018
Replace lots of hamcrest's assert_that with a builder style

Refs #5742

I've been thinking of possible solutions to close out #5742, so I'm submitting this for early feedback.

@alexcrichton what do you think? If you like the look of this I'll apply it across the suite.

r? @alexcrichton
@dwijnand
Copy link
Member

After #5942 the last remaining hamcrest usage is this kind of stuff:

    assert_that(&paths::root().join("src/lib.rs"), existing_file());
    assert_that(&paths::root().join("target/release"), existing_dir());
    assert_that(&project.bin("foo"), existing_file());
    assert_that(&res, contains(names(&["root", "foo", "bar", "baz"])));
    assert_that(&res, contains(names(&["root", "foo", "bar"])));
    assert_that(&res, is_not(contains(names(&[("util", "1.0.0")]))));
    assert_that(&res, is_not(contains(names(&[("util", "1.0.1")]))));
    assert_that(&res, is_not(contains(names(&[("util", "1.1.1")]))));
    assert_that(&res, is_not(contains(names(&[("util", "1.2.2")]))));
    assert_that(&t1, has_installed_exe("foo"));
    assert_that(&t2, has_installed_exe("foo"));
    assert_that(&t2, is_not(has_installed_exe("foo")));
    assert_that(&t3, has_installed_exe("foo"));
    assert_that(&t3, is_not(has_installed_exe("foo")));
    assert_that(&t4, has_installed_exe("foo"));
    assert_that(&t4, is_not(has_installed_exe("foo")));
    assert_that(&test.join("Cargo.toml"), is_not(existing_file()));
    assert_that(cargo_home(), has_installed_exe("bar"));
    assert_that(cargo_home(), has_installed_exe("foo"));
    assert_that(cargo_home(), has_installed_exe("foo_1"));
    assert_that(cargo_home(), has_installed_exe("foo_2"));
    assert_that(cargo_home(), is_not(has_installed_exe("bar")));
    assert_that(cargo_home(), is_not(has_installed_exe("foo")));
    assert_that(cargo_home(), is_not(has_installed_exe("foo_1")));
    assert_that(cargo_home(), is_not(has_installed_exe("foo_2")));

Any preferences on what to use in place of hamcrest?

bors added a commit that referenced this issue Aug 29, 2018
@alexcrichton
Copy link
Member Author

I'd probably just switch most of those just plain old assert! statements, no need to be too too fancy!

bors added a commit that referenced this issue Aug 29, 2018
Remove Cargo's internal `hamcrest` module

Fixes #5742
bors added a commit that referenced this issue Aug 30, 2018
Introduce the CWD macro in test output asserting

Avoids dealing with things like CWD changing.

Mentioned in #5742
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing-cargo-itself Area: cargo's tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants