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

Add tests to rustbuild #48913

Merged
merged 13 commits into from
Apr 4, 2018
Merged

Add tests to rustbuild #48913

merged 13 commits into from
Apr 4, 2018

Conversation

Mark-Simulacrum
Copy link
Member

In order to run tests, we cfg out various parts of rustbuild. Generally
speaking, these are filesystem and process-spawning operations. Then, rustbuild
is run "as normal" and the various steps that where run are retrieved from the
cache and checked against the expected results.

Note that this means that the current implementation primarily tests "what" we
build, but doesn't actually test that what we build will build. In other
words, it doesn't do any form of dependency verification for any crate. This is
possible to implement, but is considered future work.

This implementation strives to cfg out as little code as possible; it also does
not currently test anywhere near all of rustbuild. The current tests are also
not checked for "correctness," rather, they simply represent what we do as of
this commit, which may be wrong.

Test cases are drawn from the old implementation of rustbuild, though the
expected results may vary.

r? @alexcrichton

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2018
@bors
Copy link
Contributor

bors commented Mar 11, 2018

☔ The latest upstream changes (presumably #48599) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum
Copy link
Member Author

Rebased and fixed tidy.

@bors
Copy link
Contributor

bors commented Mar 11, 2018

☔ The latest upstream changes (presumably #48549) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum
Copy link
Member Author

Rebased again.

@alexcrichton
Copy link
Member

Taking a look at this now, nice!

I'm personally hesitant to land though due to the sprinkling of test-related cfg! macros and such, though. This seems like it'd be difficult to maintain over time and be a source of pain for changing rustbuild as well. (aka what if one of these files needs to be valid?).

Could we perhaps explore an alternate solution where something like run is stubbed out at a very low level? Ideally the testing here would be as least invasive as possible. Unfortunately though the design here is sort of inherently not-easily-testable...

@Mark-Simulacrum
Copy link
Member Author

Stubbing out run isn't really practical as we call ensure from it (which is pretty much required for the testing design here, and I think any viable testing design). We could explore an alternate design that separated out ensure calls from the run method but I don't personally think that would work very well with how rustbuild is designed today.

I'm personally hesitant to land though due to the sprinkling of test-related cfg! macros and such, though. This seems like it'd be difficult to maintain over time and be a source of pain for changing rustbuild as well.

I agree with this in general but I think the overhead/pain is somewhat worth it as I feel the tests outweigh the pain caused by having the cfg!s. Ideally, we'd find some solution that wouldn't require them, but so far I haven't come up with anything that shows promise. Most of them are related directly to running executables or filesystem related operations, which both seem like something we'd basically need cfg!s for today.

Could we perhaps explore an alternate solution where something like run is stubbed out at a very low level? Ideally the testing here would be as least invasive as possible. Unfortunately though the design here is sort of inherently not-easily-testable...

I'm not entirely sure what you mean by stubbing out run, as that's mostly what this does anyway (the cfg! macros in Steps are in the steps that would be stubbed out, with basically the same relative diff).

One solution we might potentially see is pulling the ensure calls for each step out of run (and moving ensure off of builder perhaps so that we can guarantee that it isn't run from run) and then passing the results into the run method. I feel like this will get rather ugly fairly quickly due to code duplication and having to follow separate codepaths but I could see it maybe working out okay. Would you like to see me explore this route in perhaps a separate branch/PR? I don't think it'll work out well but we could see, I guess.

@alexcrichton
Copy link
Member

Oh sorry I meant stubbing out run for processes instead of steps which should hopefully be lower down and the costliest part of rustbuild mostly. I'm not sure I'd agree though that these test outweight the cfg!, in the sense that we're almost no longer testing rustbuild at that point because the code is so heavily modified in test mode. This seems like it'd cause more effort to keep tests running than to diagnose/fix regressions?

I think it's a good idea to test rustbuild but I'd want to be sure that it'd done so in a relatively non-invasive way. Stubbing out running processes at the very core would be an example, but I'm also not sure how feasible that would be unfortunately :(

Other alternate designs (like the old compute-steps-first method) are more testable but have their own drawbacks too (hence the rewrite!). I'm not sure how easily testable the declarative design we have now is going to be :(

@Mark-Simulacrum
Copy link
Member Author

Oh sorry I meant stubbing out run for processes instead of steps which should hopefully be lower down and the costliest part of rustbuild mostly.

This is true (and already done in this implementation). However, we also need/want to stub out filesystem access since we want to avoid creating files all over the build directory that are test-specific, and we ideally don't want to have to run tests single threaded or some other "solution" that avoids conflicts between tests.

I would like to note that the cfg! macros are actually fairly minimally sprinkled, I've annotated them below with why they are there. Reading through the list I think it's perhaps a little more than the ideal but actually looks fairly reasonable and maintainable -- there's very few edge cases I think.

src/bootstrap/compile.rs
692:        if cfg!(test) { // codegen backends -- we expect at least one to have been built, but we build none as we haven't run cargo
743:        if cfg!(test) { return; } // parsing filesystem path
1003:    if cfg!(test) { return Vec::new(); } // cargo is run via `Command::spawn` instead of traditional APIs

src/bootstrap/lib.rs
447:        if !cfg!(test) { // parsing command output, will panic if unexpected output
675:        if cfg!(test) { return; } // running command
682:        if cfg!(test) { return; } // running command
691:        if cfg!(test) { return true; } // running command
700:        if cfg!(test) { return true; } // running command
998:        if cfg!(test) { return String::from("0.1.2"); } // parsing file system

src/bootstrap/native.rs # these are all because we can't easily stub out the cc crate's run
62:        if cfg!(test) {
339:        if cfg!(test) {
395:        if cfg!(test) {
450:        if cfg!(test) {

src/bootstrap/util.rs
213:    if cfg!(test) { return Ok(()); } # the windows code uses low-level APIs that we can't really stub out

@bors
Copy link
Contributor

bors commented Mar 16, 2018

☔ The latest upstream changes (presumably #48097) made this pull request unmergeable. Please resolve the merge conflicts.

@shepmaster
Copy link
Member

Ping from triage, @alexcrichton — will you have time to continue your review soon?

@shepmaster
Copy link
Member

@Mark-Simulacrum you have some conflicts as well.

@Mark-Simulacrum
Copy link
Member Author

I probably won't rebase and hope to discuss this at the all-hands with @alexcrichton, so we might hold off until then.

@alexcrichton
Copy link
Member

I'm down with that!

@Mark-Simulacrum
Copy link
Member Author

I've reimplemented and pushed a new version that goes with the dry_run approach I've discussed with @alexcrichton, so I think this is ready for another review pass.

@alexcrichton
Copy link
Member

Looks great! r=me with travis passing

@Mark-Simulacrum
Copy link
Member Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Apr 1, 2018

📌 Commit 5556def has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 1, 2018
@bors
Copy link
Contributor

bors commented Apr 1, 2018

⌛ Testing commit 5556def81df8062a7553ad3a9668b71d2eb48445 with merge e640148acd9eacd3095d682dd8d2370ae564420f...

@bors
Copy link
Contributor

bors commented Apr 1, 2018

💔 Test failed - status-appveyor

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 1, 2018
This ensures that each build will support the testing design of "dry
running" builds. It's also checked that a dry run build is equivalent
step-wise to a "wet" run build; the graphs we generate when running are
directly compared node/node and edge/edge, both for order and contents.
This is too likely to cause spurious bounces on CI; what we run may be
dependent on what ran successfully before hand (e.g. RLS features with
Clippy), which makes this not tenable. There's no good way to ignore
specifically these problematic steps so we'll just ignore everything for
the time being. We still test that a dry run worked though so largely
this is the same from a ensure-that-tests-work perspective.

Eventually we'll want to undo this commit, though, to make our tests
more accurate.
@Mark-Simulacrum
Copy link
Member Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Apr 3, 2018

📌 Commit 184d3bc has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 3, 2018
@kennytm
Copy link
Member

kennytm commented Apr 4, 2018

@bors p=75

@bors
Copy link
Contributor

bors commented Apr 4, 2018

⌛ Testing commit 184d3bc with merge 52af892...

bors added a commit that referenced this pull request Apr 4, 2018
Add tests to rustbuild

In order to run tests, we cfg out various parts of rustbuild. Generally
speaking, these are filesystem and process-spawning operations. Then, rustbuild
is run "as normal" and the various steps that where run are retrieved from the
cache and checked against the expected results.

Note that this means that the current implementation primarily tests "what" we
build, but doesn't actually test that what we build *will* build. In other
words, it doesn't do any form of dependency verification for any crate. This is
possible to implement, but is considered future work.

This implementation strives to cfg out as little code as possible; it also does
not currently test anywhere near all of rustbuild. The current tests are also
not checked for "correctness," rather, they simply represent what we do as of
this commit, which may be wrong.

Test cases are drawn from the old implementation of rustbuild, though the
expected results may vary.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Apr 4, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 4, 2018
@TimNN
Copy link
Contributor

TimNN commented Apr 4, 2018

Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN.

1 similar comment
@TimNN
Copy link
Contributor

TimNN commented Apr 4, 2018

Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN.

@kennytm
Copy link
Member

kennytm commented Apr 4, 2018

@bors retry

3 hour timeout in x86_64-gnu-incremental. The log has been corrupted by Travis.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2018
@bors
Copy link
Contributor

bors commented Apr 4, 2018

⌛ Testing commit 184d3bc with merge 17fea66...

bors added a commit that referenced this pull request Apr 4, 2018
Add tests to rustbuild

In order to run tests, we cfg out various parts of rustbuild. Generally
speaking, these are filesystem and process-spawning operations. Then, rustbuild
is run "as normal" and the various steps that where run are retrieved from the
cache and checked against the expected results.

Note that this means that the current implementation primarily tests "what" we
build, but doesn't actually test that what we build *will* build. In other
words, it doesn't do any form of dependency verification for any crate. This is
possible to implement, but is considered future work.

This implementation strives to cfg out as little code as possible; it also does
not currently test anywhere near all of rustbuild. The current tests are also
not checked for "correctness," rather, they simply represent what we do as of
this commit, which may be wrong.

Test cases are drawn from the old implementation of rustbuild, though the
expected results may vary.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Apr 4, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants