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.

Sign up for GitHub

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

Jump to bottom

test cleanup: Remove the argument from the project function #5746

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

test cleanup: Remove the argument from the project function #5746

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

Comments

@alexcrichton
Copy link
Member

This argument can be "foo" 99.9% of the time, so let's remove it as an argument and simply hardcode it to "foo"!
@dwijnand
Copy link
Member

Baited by the 99.9% I've started to take this on.

(Turns out it's more like 80%..

21:31:28 $ rg --stats 'project\("foo"\)' tests/ | tail -4
962 matched lines
66 files contained matches
85 files searched
0.005 seconds

21:31:38 $ rg --stats 'project\("' tests/ | tail -4
1208 matched lines
72 files contained matches
85 files searched
0.006 seconds

😄)

@alexcrichton
Copy link
Member Author

Heh true! I think in basically all cases though it can be "foo" without breaking tests

@Mark-Simulacrum
Copy link
Member

Sure, though some tests are specifically testing for things like core or syntax and making sure those work (i.e., lack of sysroot conflicts).

@dwijnand
Copy link
Member

Yeah I had a look at the outliers and some deviate without a great reason, but there are a number of them that do have a good reason. Like workspace projects, or any kind of "foo" project and "bar" project.

Anyways, after landing the change to remove the argument from the project function, I'll follow it up with a more curated change to make them use the default "foo".

@dwijnand dwijnand mentioned this issue Jul 20, 2018
Merged
bors added a commit that referenced this issue Jul 20, 2018
@bors
Auto merge of #5752 - dwijnand:project_at, r=alexcrichton
8482505 
Remove the argument from the `project` test support function

By rewriting the tests, with rerast (https://github.com/google/rerast), to
use the newly introduced "at" method.

First I added the following temporary function to cargotest::support:

    pub fn project_foo() -> ProjectBuilder {
       project("foo")
    }

Then I defined the following rewrite.rs:

    use cargotest::support::{ project, project_foo };

    fn rule1(a: &'static str) {
       replace!(project("foo") => project_foo());
       replace!(project(a) => project_foo().at(a));
    }

Then I ran rerast:

    cargo +nightly rerast --rules_file=rewrite.rs --force --targets tests --file tests/testsuite/main.rs

Finally I searched and replaced the references to project_foo with
argument-less project (a little awkardly on macOS with a git clean).

    find tests -type f -exec sed -i -e 's/project_foo/project/g' {} +
    git clean -d tests

Fixes #5746
@dwijnand dwijnand mentioned this issue Jul 21, 2018
Merged
bors added a commit that referenced this issue Jul 21, 2018
@bors
Auto merge of #5761 - dwijnand:project-food, r=alexcrichton
207c2cb 
Rework some test projects to use the "foo" default

Generally that means either switching "foo" and "bar" around (reversing
the arrow), or it means push "foo" to "bar" (and sometimes "bar" to
"baz", etc..) to free up "foo".

For trivia that leaves 80/1222 outliers, therefore 93.4% of test
project use the default. :)

Refs #5746
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-testing-cargo-itself Area: cargo's tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove the argument from the project test support function dwijnand/cargo
3 participants
@alexcrichton @dwijnand @Mark-Simulacrum