Stream: t-compiler/cargo-bisect-rustc

Topic: Testing


Chris Simpkins (Feb 25 2020 at 18:42, on Zulip):

Moving this to a new thread from Release New Version so that we can continue the conversation

pnkfelix said:

wishes we had a test suite.

pnkfelix said:

actually I guess it wouldn't be that hard to make a test suite, if we're willing to use --end for all the tests, and just cross our finges that the behavior without --end does not break.

Chris Simpkins said:

I've been thinking through how to approach this. The commit level rustc caches expire at the 167 day mark so the tests covering this part of the source would need to be updated periodically.

pnkfelix said:

hmm. that is a good point.
we might at least be able to make the update process relatively painless, by using rustversion and #[rustc_error]...

pnkfelix (Feb 25 2020 at 19:34, on Zulip):

another option would be to just not attempt to test the commit level caches... :(

pnkfelix (Feb 25 2020 at 19:35, on Zulip):

or maybe we could find out if we could ... "bless" certain commits to live beyond the 167 day expiration period, solely for the purpose of supporting the test suite? That might be the simplest option here...

pnkfelix (Feb 25 2020 at 19:35, on Zulip):

@simulacrum any thought on the latter approach?

simulacrum (Feb 25 2020 at 19:42, on Zulip):

I'd just test on nightlies for now, blessing is possible but a bit painful i think

simulacrum (Feb 25 2020 at 19:45, on Zulip):

Or just updated the test once a year ish

simulacrum (Feb 25 2020 at 19:45, on Zulip):

Which might be reasonable

simulacrum (Feb 25 2020 at 19:45, on Zulip):

To test new things

pnkfelix (Feb 25 2020 at 19:49, on Zulip):

wouldn't it would need to be roughly twice a year? (365 / 167 \approx 2?)

pnkfelix (Feb 25 2020 at 19:51, on Zulip):

(and really this is the kind of thing where you need to round up, not down. :slight_smile: so ⌈365/167⌉ = 3 ...

pnkfelix (Feb 25 2020 at 19:51, on Zulip):

nonetheless, updating the tests three times a year probably is better than blessing some random commits

Chris Simpkins (Feb 25 2020 at 20:46, on Zulip):

We just need to make sure that there is at least one good regression every six months... :slight_smile:

simulacrum (Feb 25 2020 at 21:44, on Zulip):

yes, I would personally lean towards updating the test I think for now at least, long-term I think we may want to eventually have commit artifacts stick around even longer -- e.g., we'd preserve every 5th commit after 167 days

simulacrum (Feb 25 2020 at 21:51, on Zulip):

one note: we could "fake" commit bisection in theory for the tests

simulacrum (Feb 25 2020 at 21:51, on Zulip):

i.e., we separately test that we can download commits for example

simulacrum (Feb 25 2020 at 21:51, on Zulip):

but then for the actual test of commit bisection we provide a custom "url generator" or "downloader" impl that actually downloads nightlies behind the back of the commit bisection

simulacrum (Feb 25 2020 at 21:52, on Zulip):

which are, at least in theory, "just" sparse commits

simulacrum (Feb 25 2020 at 21:52, on Zulip):

i.e. everything's a rollup

Chris Simpkins (Feb 25 2020 at 23:09, on Zulip):

Could we mock the regression check part of execution with a handful of strategically designed executables in a repository that is designed to have a series of git commits in a ‘nightly’ and a roll up PR. ‘rustc’ could even be scripts that just spit out the proper exit status codes and stderr messages for the tests.

pnkfelix (Feb 26 2020 at 04:07, on Zulip):

simulacrum said:

yes, I would personally lean towards updating the test I think for now at least, long-term I think we may want to eventually have commit artifacts stick around even longer -- e.g., we'd preserve every 5th commit after 167 days

Or maybe go with a exponentially decaying archive: after 167 days, keep the 2nd, 4th, 8th, 16th, 32nd, ... commits

pnkfelix (Feb 26 2020 at 04:08, on Zulip):

(I am mostly kidding; we're keeping the nightlies anyway)

pnkfelix (Feb 26 2020 at 04:09, on Zulip):

((and hopefully someone will keep paying for that expanding storage in perpetuity ....))

simulacrum (Feb 26 2020 at 14:03, on Zulip):

yeah, I think the main reason we've not done it is it's somewhat non-trivial to do and not really worthwhile

pnkfelix (Mar 02 2020 at 21:11, on Zulip):

okay after poking around a bit, I found a crate rustversion that used a build.rs script to inspect the rustc version. I tried using that crate directly, but it takes too long to repeatedly compile it and its depedencies during bisection. But the strategy itself is easily adapted.

pnkfelix (Mar 02 2020 at 21:11, on Zulip):

I hope to have a PR with some demo tests up in a bit.

Chris Simpkins (Mar 07 2020 at 19:34, on Zulip):

It looks like this is a "stable" ICE for testing? :slight_smile: => https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=0ab2bd6a9d722e0f05a95e2a5dcf89cc

Chris Simpkins (Mar 17 2020 at 20:25, on Zulip):

pnkfelix said:

okay after poking around a bit, I found a crate rustversion that used a build.rs script to inspect the rustc version. I tried using that crate directly, but it takes too long to repeatedly compile it and its depedencies during bisection. But the strategy itself is easily adapted.

@pnkfelix Do you have more details about how you intend to approach this?

pnkfelix (Mar 18 2020 at 12:21, on Zulip):

Yeah i have a demo in a branch

pnkfelix (Apr 06 2020 at 20:09, on Zulip):

sorry that I didn't follow up about this

pnkfelix (Apr 06 2020 at 20:09, on Zulip):

here is the branch with WIP demo: https://github.com/rust-lang/cargo-bisect-rustc/compare/master...pnkfelix:add-cli-test-frameworkl?expand=1

Chris Simpkins (Apr 06 2020 at 20:33, on Zulip):

No worries! Will check it out Felix. Thanks

Chris Simpkins (May 12 2020 at 16:06, on Zulip):

@t-compiler/cargo-bisect-rustc Picking up the conversation on bisect-rustc testing again in light of the discussion in https://rust-lang.zulipchat.com/#narrow/stream/217417-t-compiler.2Fcargo-bisect-rustc/topic/Searched.20date.20range.20misreported.20.2382. Is there time/interest to pitch in on an effort to refactor the bisect-rustc source, broaden test support, and review PR's with these changes?

pnkfelix (May 12 2020 at 19:17, on Zulip):

I might be willing to try to revive that branch I made

pnkfelix (May 13 2020 at 13:50, on Zulip):

let me see if I can allocate time to do this today

pnkfelix (May 14 2020 at 00:45, on Zulip):

https://github.com/rust-lang/cargo-bisect-rustc/pull/88

Chris Simpkins (May 15 2020 at 05:21, on Zulip):

Added GNU/Linux, macOS, Win GH Actions CI workflows in https://github.com/rust-lang/cargo-bisect-rustc/pull/90

Santiago Pastorino (May 15 2020 at 13:55, on Zulip):

@Chris Simpkins looks good, should we merge this now or wait for cargo-bisect-rustc#89 ?

Santiago Pastorino (May 15 2020 at 13:57, on Zulip):

also about cargo-bisect-rustc#89 we can temporarily fix on our side by adding that dependency bump in our Cargo.toml

Chris Simpkins (May 15 2020 at 13:57, on Zulip):

I think that it may be helpful for @pnkfelix to work on his CI testing PR. That was the goal at least... :slight_smile:. As long as Felix doesn't mind, I think that we can merge it

Santiago Pastorino (May 15 2020 at 13:57, on Zulip):

yeah my main concern is cargo-bisect-rustc#89 won't this make all the future PRs fail unless the PR is to fix that problem?

Chris Simpkins (May 15 2020 at 14:01, on Zulip):

Fix just submitted in https://github.com/rust-lang/cargo-bisect-rustc/pull/91

Santiago Pastorino (May 15 2020 at 14:02, on Zulip):

both merged

Santiago Pastorino (May 15 2020 at 14:03, on Zulip):

Chris Simpkins said:

Fix just submitted in https://github.com/rust-lang/cargo-bisect-rustc/pull/91

I guess it's better to make the bump in Cargo.toml but anyway, if they will fix this on console we would need to later revert the Cargo.toml change so ... meh

pnkfelix (May 15 2020 at 15:45, on Zulip):

I'm sorry that I haven't been following everything that's been going on, but: did --regress=ice support break at some point in the past month or so?

pnkfelix (May 15 2020 at 15:45, on Zulip):

I'm a little surprised by the behavior I'm seeing from cargo-bisect-rustc on one of my test cases, and I thought I should do some sanity checking first...

pnkfelix (May 15 2020 at 15:47, on Zulip):

hmm I wonder if this is due to https://github.com/rust-lang/cargo-bisect-rustc/pull/78 ...

Santiago Pastorino (May 15 2020 at 16:02, on Zulip):

@pnkfelix maybe ...

Santiago Pastorino (May 15 2020 at 16:02, on Zulip):

do you have the app you're trying available?

Santiago Pastorino (May 15 2020 at 16:02, on Zulip):

if you have I can test it out

Santiago Pastorino (May 15 2020 at 16:02, on Zulip):

and fix the issues we may be hitting

pnkfelix (May 15 2020 at 16:04, on Zulip):

the app is the "eventually_ice" test from https://github.com/rust-lang/cargo-bisect-rustc/pull/88

pnkfelix (May 15 2020 at 16:04, on Zulip):

the basic problem

pnkfelix (May 15 2020 at 16:04, on Zulip):

is that https://github.com/rust-lang/cargo-bisect-rustc/pull/78 assumes that error code 101 suffices to identify ICE's

pnkfelix (May 15 2020 at 16:05, on Zulip):

and that is... bogus?

pnkfelix (May 15 2020 at 16:05, on Zulip):

namely, I think even normal compiler errors return error code 101 from cargo?

pnkfelix (May 15 2020 at 16:06, on Zulip):

basically, I think that any crate that has a "normal error" on version A of rustc, and an ICE on version B of rustc, will fail when you try to run --regress=ice

pnkfelix (May 15 2020 at 16:07, on Zulip):

because now cargo-bisect-rustc is incorrectly thinking that version A causes an ICE, and thus reports that the start commit is not allowed to reproduce the regression.

Santiago Pastorino (May 15 2020 at 16:07, on Zulip):

:+1:, do you have a fix for this or we should just revert?

pnkfelix (May 15 2020 at 16:07, on Zulip):

I think the easy fix is to resume processing stderr for the cases where we are getting an error code

pnkfelix (May 15 2020 at 16:08, on Zulip):

but the main issue is that other kinds of crashes still need to be identified

pnkfelix (May 15 2020 at 16:08, on Zulip):

let me see what I can do

pnkfelix (May 15 2020 at 16:08, on Zulip):

eh, simplest maybe to just revert it

pnkfelix (May 15 2020 at 16:08, on Zulip):

land my test

pnkfelix (May 15 2020 at 16:09, on Zulip):

and then see about re-adding the functionality of https://github.com/rust-lang/cargo-bisect-rustc/pull/78 in a way that does not break my test

Santiago Pastorino (May 15 2020 at 16:09, on Zulip):

do you want me to do this, I meant, revert and explain in the PR?

pnkfelix (May 15 2020 at 16:10, on Zulip):

I'll take care of it

pnkfelix (May 15 2020 at 16:10, on Zulip):

I'll make it part of the PR that adds my test

Santiago Pastorino (May 15 2020 at 16:10, on Zulip):

ok, it's not a problem for me but :+1:

Santiago Pastorino (May 15 2020 at 16:10, on Zulip):

cool, going to work a bit on pre-prioritization automation then :)

pnkfelix (May 15 2020 at 16:10, on Zulip):

unless you think there is a strong reason to make the revert part of a separate PR?

Santiago Pastorino (May 15 2020 at 16:10, on Zulip):

I don't think so

pnkfelix (May 15 2020 at 16:44, on Zulip):

hmm. Glacier seems to think that normal errors will non-101 (and non-zero) error codes.

pnkfelix (May 15 2020 at 16:47, on Zulip):

I wonder if this is something where rustc produces non-101 but cargo produces 101?

Chris Simpkins (May 15 2020 at 17:56, on Zulip):

What is the issue with rustfmt Felix?

Chris Simpkins (May 15 2020 at 18:09, on Zulip):

Also, do we want to push the Windows build fix today or wait on the ICE definition to be addressed?

pnkfelix (May 18 2020 at 16:32, on Zulip):

Chris Simpkins said:

What is the issue with rustfmt Felix?

It is some bug with how it deals with mod inner { mod file1; mod file2; } within a #[test] the tests/ directory for a crate

pnkfelix (May 18 2020 at 16:32, on Zulip):

Its already logged as an issue on the rustfmt repository

pnkfelix (May 18 2020 at 16:33, on Zulip):

(and I linked to it from my PR; its rust-lang/rustfmt#3794 )

Chris Simpkins (May 23 2020 at 16:58, on Zulip):

@pnkfelix want to push the ICE definition changes (revert recent ICE definition changes) in a separate PR/new release before we merge the testing updates? https://github.com/rust-lang/cargo-bisect-rustc/pull/88/commits/7bd1ec404ff28377f2bfd2652fa86edea997f7e0

pnkfelix (May 23 2020 at 22:58, on Zulip):

Yeah @Chris Simpkins let’s do that, given i cannot figure out why my PR is currently otherwise blocked

Chris Simpkins (May 24 2020 at 22:09, on Zulip):

pnkfelix said:

Yeah Chris Simpkins let’s do that, given i cannot figure out why my PR is currently otherwise blocked

https://github.com/rust-lang/cargo-bisect-rustc/pull/94

Chris Simpkins (May 26 2020 at 15:05, on Zulip):

Chris Simpkins said:

pnkfelix said:

Yeah Chris Simpkins let’s do that, given i cannot figure out why my PR is currently otherwise blocked

https://github.com/rust-lang/cargo-bisect-rustc/pull/94

@pnkfelix OK to merge and release this?

pnkfelix (May 26 2020 at 15:06, on Zulip):

I hit merge just a minute or two ago. I'm cool with a release

pnkfelix (May 26 2020 at 15:06, on Zulip):

I may or may not have time to look into the CLI testing PR's issues today. Not sure how best to deal with some random reqwest issue...

Chris Simpkins (May 26 2020 at 15:06, on Zulip):

pnkfelix said:

I hit merge just a minute or two ago. I'm cool with a release

Ha! Sorry, I didn't check my notifications before I asked... Apologies

pnkfelix (May 26 2020 at 15:06, on Zulip):

no prob, it was super recent

Chris Simpkins (May 26 2020 at 15:09, on Zulip):

pnkfelix said:

I may or may not have time to look into the CLI testing PR's issues today. Not sure how best to deal with some random reqwest issue...

I need to spend some time with your source to understand what you are doing with the tests. I'm afraid that I am not much help right now b/c I don't understand it

pnkfelix (May 26 2020 at 15:09, on Zulip):

I understand that

pnkfelix (May 26 2020 at 15:10, on Zulip):

I tried to outline the two strategies in the commit message (or was it the PR description) ... but if anything in the outline itself is unclear, please do ask questions

pnkfelix (May 26 2020 at 15:10, on Zulip):

otherwise if its about the source itself, I'll admit that that is/was a little obtuse at times.

pnkfelix (May 26 2020 at 15:10, on Zulip):

at least, I knew that even I myself had to work a little bit to remember how it worked

Chris Simpkins (May 26 2020 at 15:10, on Zulip):

We'll need to figure out how to maintain the source that needs to be updated too.

pnkfelix (May 26 2020 at 15:11, on Zulip):

it would be good to have some way to track when that source is going to expire

pnkfelix (May 26 2020 at 15:11, on Zulip):

and replace it with something else ahead of time

Chris Simpkins (May 26 2020 at 15:12, on Zulip):

Yes, possible to have a ~quarterly cadence of a good ICE example from compiler meetings that we can use for the updates?

Chris Simpkins (May 26 2020 at 15:28, on Zulip):

If compiler meeting is not appropriate for tracking this, I'd be happy to add it to my personal calendar and will do my best™ . This is a less reliable approach. :slight_smile:

pnkfelix (May 26 2020 at 15:41, on Zulip):

hmm. Interesting question of who should be in charge of this. I suspect its not currently part of the T-compiler charter, as much as I'd like it to be

pnkfelix (May 26 2020 at 15:42, on Zulip):

but I certainly will be willing to put it on my calendar as well. :)

Last update: Jul 02 2020 at 12:55UTC