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 ‘#[serial]’ tests (v2) #42684

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
@shivjm
Copy link

shivjm commented Jun 15, 2017

(Fixed version of #42626; sorry for the noise.)

This PR adds a #[serial] attribute that marks tests that should be executed one-by-one rather than concurrently (per #33519).

I’ve confirmed that the libtest and tidy tests pass. I wasn’t able to run the full suite as a lot of the run_make tests seem to fail on my Windows system, but everything passed up to that point.

Rust (and PR!) newbie here, so please do let me know if I’m doing something poorly or incorrectly!

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jun 15, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@leonardo-m

This comment has been minimized.

Copy link

leonardo-m commented Jun 15, 2017

Perhaps a name like #[serial_test] is better?

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jun 15, 2017

I think #[serial] is fine given that #[test] will also be there, but I could see an argument being made that serial is too generic a name. I do think this needs a feature gate though, which I believe hasn't yet been implemented. I could be wrong though.

@shivjm

This comment has been minimized.

Copy link
Author

shivjm commented Jun 15, 2017

Ah, I didn’t understand how the feature gates work and the example I followed (#42219) is apparently a WIP. Looking at #42088, I need to gate this by:

  1. Adding an entry to the declare_features block in feature_gate.rs with a feature name, a version, and an issue number:
(active, serial_tests, "1.19.0", Some(42684))
  1. Specifying AttributeGate::Gated for the serial attribute in BUILTIN_ATTRIBUTES (same file), which needs a feature name, an explanation, and a function to check its configuration (auto-generated using cfg_fn):
("serial", Normal, Gated(Stability::Unstable, "serial_tests", "the `#[serial]` attribute is an experimental feature", cfg_fn!(serial_tests))
  1. Adding a new document under unstable-book/src/language-features/ that points to this PR.
  2. Adding an entry under unstable-book/src/SUMMARY.md that points to that document.

Is this correct? Have I missed anything? I’ll try to get it done pronto, but if not, it will be about a week.

Re: the name of the attribute itself, I can change that if serial is too generic.

@bjorn3
Copy link
Contributor

bjorn3 left a comment

Few minor things. Overal code looks good to me.

@@ -52,7 +52,8 @@ struct Test {
path: Vec<Ident> ,
bench: bool,
ignore: bool,
should_panic: ShouldPanic
should_panic: ShouldPanic,
serial: bool

This comment has been minimized.

@bjorn3

bjorn3 Jun 20, 2017

Contributor

Please end the line with a comma, so there is less noise when new fields get added.

@@ -133,7 +134,8 @@ impl<'a> fold::Folder for TestHarnessGenerator<'a> {
path: self.cx.path.clone(),
bench: is_bench_fn(&self.cx, &i),
ignore: is_ignored(&i),
should_panic: should_panic(&i, &self.cx)
should_panic: should_panic(&i, &self.cx),
serial: is_serial(&i)

This comment has been minimized.

@bjorn3

bjorn3 Jun 20, 2017

Contributor

Same here

name: StaticTestName("first"),
ignore: false,
should_panic: ShouldPanic::No,
serial: true

This comment has been minimized.

@bjorn3

bjorn3 Jun 20, 2017

Contributor

Same here

name: StaticTestName("second"),
ignore: false,
should_panic: ShouldPanic::No,
serial: true

This comment has been minimized.

@bjorn3

bjorn3 Jun 20, 2017

Contributor

Same here

TestEvent::TeFilteredOut(n) if n > 0 => panic!("filtered out"),
TestEvent::TeTimeout(_) => panic!("timeout"),
TestEvent::TeResult(_, ref result, _) if result != &TestResult::TrOk =>
panic!("result not okay"),

This comment has been minimized.

@bjorn3

bjorn3 Jun 20, 2017

Contributor

Please print the result too. Eg

panic!("result not okay: {:?}", result),
TestEvent::TeFilteredOut(n) if n > 0 => panic!("filtered out"),
TestEvent::TeTimeout(_) => panic!("timeout"),
TestEvent::TeResult(_, ref result, _) if result != &TestResult::TrOk =>
panic!("result not okay"),

This comment has been minimized.

@bjorn3

bjorn3 Jun 20, 2017

Contributor

Same printing thing here.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 21, 2017

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

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jun 25, 2017

@shivjm I believe this is waiting on some changes from you and a rebase. Let us know if we can help in any way! Feel free to ping me on IRC (simulacrum).

@shivjm

This comment has been minimized.

Copy link
Author

shivjm commented Jun 25, 2017

Sorry, been out of town and then busy catching up! I’ve rebased the PR and (I hope) fixed the issues @bjorn3 pointed out (thanks!). Next up, need to implement the feature gate. I’ll ask for help on IRC.

@shivjm

This comment has been minimized.

Copy link
Author

shivjm commented Jun 25, 2017

I’ve added the serial_tests feature gate. @est31, does this need an entry under unstable-book as well?

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Jun 26, 2017

does this need an entry under unstable-book as well?

The requirement for an entry has been removed. You can add one (with meaningful content) if you want to, if not, a stub will be generated automatically.

@bjorn3
Copy link
Contributor

bjorn3 left a comment

Lgtm when the trailing newline is added.

#[serial] //~ ERROR serial tests are currently unstable
fn in_serial() {
assert!(true);
}

This comment has been minimized.

@bjorn3

bjorn3 Jun 26, 2017

Contributor

Please add a trailing newline.

@shivjm

This comment has been minimized.

Copy link
Author

shivjm commented Jun 26, 2017

Done, thanks!

@bjorn3

bjorn3 approved these changes Jun 26, 2017

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jun 27, 2017

Thanks for the patch.

This probably needs more discussion before landing. This is a thing that we've declined to do for many years.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jun 27, 2017

cc @nrc

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 29, 2017

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

@shivjm

This comment has been minimized.

Copy link
Author

shivjm commented Jul 3, 2017

Is there anything I can do to help?

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 6, 2017

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

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jul 10, 2017

@brson do you have links to previous discussion or could you summarise here please? My opinion (uninformed by previous discussion) is that this is something I've wanted personally and I don't see why we should not support it (as long as it is feature gated etc.) being its own attribute feels kind of wrong (I'd prefer to attach it to #[test], but I suppose that #[should_panic] is a precedent?)

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jul 10, 2017

Discussed at the dev-tools meeting today, this problem can be solved today using a mutex (I filed #43155 to document that). Given that this solution feels somewhat ad hoc and there is workaround, we would prefer to punt on adding this in favour of custom test runners or some other long-term solution.

Thanks for the PR @shivjm and sorry to close it out after your work.

@shivjm

This comment has been minimized.

Copy link
Author

shivjm commented Jul 11, 2017

@yshui

This comment has been minimized.

Copy link

yshui commented Feb 21, 2018

@nrc I don't see why this should be implemented in the test framework. The mutex work around seems to have some catches, and is non-trivial to get right.

@yshui yshui unassigned brson Feb 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment