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

Simplify tests #2670

Merged
merged 2 commits into from
May 11, 2016
Merged

Simplify tests #2670

merged 2 commits into from
May 11, 2016

Conversation

matklad
Copy link
Member

@matklad matklad commented May 11, 2016

@alexcrichton I think there is one bit of tests that could be simplified.

What do you think about writing this

test!(simple {
    pkg("foo", "0.0.1");

    assert_that(cargo_process("install").arg("foo"),
                execs().with_status(0).with_stdout(&format!("\
[UPDATING] registry `[..]`
[DOWNLOADING] foo v0.0.1 (registry file://[..])
[COMPILING] foo v0.0.1 (registry file://[..])
[INSTALLING] {home}[..]bin[..]foo[..]
",
        home = cargo_home().display())));
    assert_that(cargo_home(), has_installed_exe("foo"));

    assert_that(cargo_process("uninstall").arg("foo"),
                execs().with_status(0).with_stdout(&format!("\
[REMOVING] {home}[..]bin[..]foo[..]
",
        home = cargo_home().display())));
    assert_that(cargo_home(), is_not(has_installed_exe("foo")));
});

instead of this

test!(simple {
    pkg("foo", "0.0.1");

    assert_that(cargo_process("install").arg("foo"),
                execs().with_status(0).with_stdout(&format!("\
{updating} registry `[..]`
{downloading} foo v0.0.1 (registry file://[..])
{compiling} foo v0.0.1 (registry file://[..])
{installing} {home}[..]bin[..]foo[..]
",
        updating = UPDATING,
        downloading = DOWNLOADING,
        compiling = COMPILING,
        installing = INSTALLING,
        home = cargo_home().display())));
    assert_that(cargo_home(), has_installed_exe("foo"));

    assert_that(cargo_process("uninstall").arg("foo"),
                execs().with_status(0).with_stdout(&format!("\
{removing} {home}[..]bin[..]foo[..]
",
        removing = REMOVING,
        home = cargo_home().display())));
    assert_that(cargo_home(), is_not(has_installed_exe("foo")));
});

This PR has a proof of concept implementation of this feature applied to a couple of tests.

r? @alexcrichton

@alexcrichton
Copy link
Member

Sounds good to me! I like the idea of making these more ergonomic :)

@alexcrichton
Copy link
Member

Wanna tackle some more as part of this?

@matklad
Copy link
Member Author

matklad commented May 11, 2016

Wanna tackle some more as part of this?

Not sure what did you mean by this :( I want to change all the tests and probably to get rid of those statics and that would be all for this PR. substitute_macros fn does a horrible amount of useless coping, but I it should not matter performance-wise.

@alexcrichton
Copy link
Member

Ah yeah that's all I mean, applying this treatment to more tests. And yeah so long as the tests don't take days it's fine by me :)

@matklad
Copy link
Member Author

matklad commented May 11, 2016

Ok, I've removed about a third of this formatting constants. I hope to continue this work tomorrow, but it might be a good idea to merge the current work:

  • it already saves 250 lines of code
  • it will conflict with basically any other change to tests.

@alexcrichton
Copy link
Member

I'd be fine with an incremental approach, but looks like tidy is failing?

remove RUNNING, COMPILING, ERROR, DOCUMENTING, FRESH and UPDATING
constants
@matklad
Copy link
Member Author

matklad commented May 11, 2016

but looks like tidy is failing?

Ah, I've just cargo tested it locally, without ./test/check-style.sh. Updated the PR.

And indeed I am afraid that I can introduce a bit of bad formatting, because most changes here are done via regex search/replace (no structural search/replace in IntelliJ-Rust yet :( ). But in my opinion these tests are not exactly beautiful anyway :)

@alexcrichton
Copy link
Member

@bors: r+ 29cdad4

@bors
Copy link
Contributor

bors commented May 11, 2016

⌛ Testing commit 29cdad4 with merge e3bc030...

bors added a commit that referenced this pull request May 11, 2016
Simplify tests

@alexcrichton I think there is one bit of tests that could be simplified.

What do you think about writing this

```Rust
test!(simple {
    pkg("foo", "0.0.1");

    assert_that(cargo_process("install").arg("foo"),
                execs().with_status(0).with_stdout(&format!("\
[UPDATING] registry `[..]`
[DOWNLOADING] foo v0.0.1 (registry file://[..])
[COMPILING] foo v0.0.1 (registry file://[..])
[INSTALLING] {home}[..]bin[..]foo[..]
",
        home = cargo_home().display())));
    assert_that(cargo_home(), has_installed_exe("foo"));

    assert_that(cargo_process("uninstall").arg("foo"),
                execs().with_status(0).with_stdout(&format!("\
[REMOVING] {home}[..]bin[..]foo[..]
",
        home = cargo_home().display())));
    assert_that(cargo_home(), is_not(has_installed_exe("foo")));
});
```

instead of this

```Rust
test!(simple {
    pkg("foo", "0.0.1");

    assert_that(cargo_process("install").arg("foo"),
                execs().with_status(0).with_stdout(&format!("\
{updating} registry `[..]`
{downloading} foo v0.0.1 (registry file://[..])
{compiling} foo v0.0.1 (registry file://[..])
{installing} {home}[..]bin[..]foo[..]
",
        updating = UPDATING,
        downloading = DOWNLOADING,
        compiling = COMPILING,
        installing = INSTALLING,
        home = cargo_home().display())));
    assert_that(cargo_home(), has_installed_exe("foo"));

    assert_that(cargo_process("uninstall").arg("foo"),
                execs().with_status(0).with_stdout(&format!("\
{removing} {home}[..]bin[..]foo[..]
",
        removing = REMOVING,
        home = cargo_home().display())));
    assert_that(cargo_home(), is_not(has_installed_exe("foo")));
});
```

This PR has a proof of concept implementation of this feature applied to a couple of tests.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented May 11, 2016

@bors bors merged commit 29cdad4 into rust-lang:master May 11, 2016
@matklad matklad mentioned this pull request May 12, 2016
bors added a commit that referenced this pull request May 12, 2016
Simplify more tests

This is the followup of #2670

It contains [one](28887be#diff-ebcf8bfb935037902f135a7c225479b5L322) more or less significant change, everything else is just search/replace.

r? @alexcrichton
@matklad matklad deleted the simplify-tests branch June 9, 2016 10:56
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