Replace lots of hamcrest's assert_that with a builder style

[dwijnand]

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

[Eh2406]

Overall I like the direction. mabe add a #[must_use] to Execs?

[dwijnand]

I'm copying Alex's implementation in rustfix: https://github.com/rust-lang-nursery/rustfix/blob/116c7e9f7b666d92c049ab86c1d83e8ced1cead4/cargo-fix/tests/all/main.rs#L323-L329

How do the two techniques compare?

[dwijnand]

Interesting: ProjectBuilder already has the must_use attribute applied to it.

[Eh2406]

Defence in depth, use both. One checks at runtime the other at compile time.

[alexcrichton]

Looking good to me! I wonder if we could lift the requirement though of requiring both execs and run? Having both feels slightly redundant (but I think one is good to keep)

[dwijnand]

Yeah we can make .cargo return Execs and then forward the ProcessBuilder methods used in the test suite.

How do you feel, instead, about running the assert in the Drop? It would remove the need to expressly execute .run() but maybe it's a bad practice? .run() would still be there when you want to force the execution order. I'm torn 😄

[alexcrichton]

Sounds plausible to me! (having .cargo return Execs)

I'd be more wary of removing the need for .run() in the sense that having such significant behavior happen in a destructor can be a bit scary for codegen reasons here and there.

[alexcrichton]

Looks great to me!

[dwijnand]

This doesn't completely remove hamcrest, but it does a lot.

I'm concerned that my grand refactorings will clash with parallel development in PRs, so I'd love to land this PR into master and send follow up PRs to close out #5742.

[dwijnand]

Could someone restart AppVeyor please?

[Eh2406]

Closing and reopening will trigger a retest, or pushing more commits.

[dwijnand]

Yeah but both are noisy.

That said my commenting is also noisy so I'm not doing any better..

[alexcrichton]

Holy cow, nice! Thanks so much for taking this on!

Let's definitely get this in before it bitrots!

@bors: r+

[bors]

📌 Commit 85984a8 has been approved by alexcrichton

[bors]

⌛ Testing commit 85984a8 with merge 9ddf75a...

[dwijnand]

Cheers, Alex. 🍻

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 9ddf75a to master...

[Eh2406]

And another net negative PR. If you keep up with this the project will just disappear. :-P