Make tidy check for lang gate tests #38914

est31 · 2017-01-08T03:09:03Z

Add gate tests to the checks that tidy performs. Excerpt from the commit message of the main commit:

Require compile-fail tests for new lang features

Its non trivial to test lang feature gates, and people
forget to add such tests. So we extend the features lint
of the tidy tool to ensure that all new lang features
contain a new compile-fail test.

Of course, one could drop this requirement and just
grep all tests in run-pass for #![feature(abc)] and
then run this test again, removing the mention,
requiring that it fails.

But this only tests for the existence of a compilation
failure. Manual tests ensure that also the correct lines
spawn the error, and also test the actual error message.

For library features, it makes no sense to require such
a test, as here code is used that is generic for all
library features.

The tidy lint extension now checks the compile-fail test suite for occurences of "gate-test-X" where X is a feature. Alternatively, it also accepts file names with the form "feature-gate-X.rs". If a lang feature is found that has no such check, we emit a tidy error.

I've applied the markings to all tests I could find in the test suite. I left a small (20 elements) whitelist of features that right now have no gate test, or where I couldn't find one. Once this PR gets merged, I'd like to close issue #22820 and open a new one on suggestion of @nikomatsakis to track the removal of all elements from that whitelist (already have a draft). Writing such a small test can be a good opportunity for a first contribution, so I won't touch it (let others have the fun xD).

cc @brson , @pnkfelix (they both discussed about this in the issue linked above).

rust-highfive · 2017-01-08T03:09:14Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

nikomatsakis · 2017-01-10T18:23:43Z

src/tools/tidy/src/features.rs

+                                .count();
+
+    // FIXME get this number down to zero.
+    let gate_untested_expected = 94;


Maybe we can open up a follow-up issue with a list of each of these tests and try to distribute the work.

Sure, good idea! Most of the features already have gate tests (see issue #22820 for an outdated list). If the general direction of this issue is agreed upon, I volunteer to add // gate-test- comments to the already existing tests (that I can find), and for the small remaining number we can open a tracking issue. Probably a good thing to ask contributions for from first time contributors (like the error message refactoring).

nikomatsakis · 2017-01-10T18:23:53Z

@bors r+

bors · 2017-01-10T18:23:54Z

📌 Commit 9d04411 has been approved by nikomatsakis

nikomatsakis · 2017-01-10T18:24:13Z

@bors r-

nikomatsakis · 2017-01-10T18:25:28Z

Actually, one thing -- I think we should edit COMPILER_TESTS.md to document what is expected. It'd be nice if the error message also directed the user at some instructions on how to fix (or directly gave those instructions).

nikomatsakis · 2017-01-10T18:26:21Z

src/tools/tidy/src/features.rs

+        if gate_untested < gate_untested_expected {
+            println!("Did you forget to reduce the expected number?");
+        } else {
+            println!("Did you forget to add a gate test for your new feature?");


Also, I think we should make the "expected" list be a hash set or something. This way we can actually print out the names of the features here, along with some directions like "make a test with a // gate-test-foo comment"

This is not the aspired end state. I'm sure most features already have gate tests, there should only be very few without one. And once we have all features gate tested, so gate_untested_expected is zero, we can change this code to print the ungated features in the error message.

nikomatsakis

I think the idea is very good but I feel like the experience of getting a tidy error here will be kind of annoying. With a bit more effort we can help direct people as to what they have to do.

est31 · 2017-01-10T20:33:32Z

I feel like the experience of getting a tidy error here will be kind of annoying

See my reply above. The tidy error will/can be improved once the list is complete.

nikomatsakis · 2017-01-10T22:06:17Z

@est31

The tidy error will/can be improved once the list is complete.

Hmm. That seems potentially OK, but I am concerned we will stall in this interim state for some time. =)

est31 · 2017-01-11T01:37:55Z

Okay, updated, and docs added.

I've now added a whitelist of currently 20 features for which I couldn't find gate tests.

They either don't have a test, or are already removed, but feature_gate.rs didn't get updated,
or other issues. Once the PR is merged I'd like to open a tracking issue with the different categories the tests fall into.

Thanks to the whitelist, we can now fail with messages like:

Expected a gate test for the feature 'type_ascription'.
Hint: create a file named 'feature-gate-type_ascription.rs' in the compile-fail
      test suite, with its failures due to missing usage of
      #![feature(type_ascription)].
Hint: If you already have such a test and don't want to rename it,
      you can also add a // gate-test-type_ascription line to the test file.
Found 1 features without a gate test.

est31 · 2017-01-11T01:38:08Z

r? @nikomatsakis

est31 · 2017-01-11T01:52:32Z

Oh sorry, forgot to push. Now it should be okay. r? @nikomatsakis

est31 · 2017-01-11T03:02:00Z

Updated PR description.

nikomatsakis · 2017-01-11T16:47:29Z

src/tools/tidy/src/features.rs

+    let gate_untested = features.iter()
+                                .filter(|&(_, f)| f.level == Status::Unstable)
+                                .filter(|&(_, f)| !f.has_gate_test)
+                                .filter(|&(n, _)| !whitelist.contains(&n.as_str()))


I'm happy to land it as is, but I have a mild concern that people will add tests for something in the whitelist, but forget to remove it from the whitelist. Either we have to be vigilant, or we could tweak this code such that if it is in the whitelist, we error out if it is found to be tested.

nikomatsakis · 2017-01-11T16:47:43Z

@bors r+

bors · 2017-01-11T16:47:44Z

📌 Commit 66718ed has been approved by nikomatsakis

nikomatsakis · 2017-01-11T16:48:12Z

I'll enqueue it for now, can always submit a follow-up PR if you think it's worth tweaking it.

Its non trivial to test lang feature gates, and people forget to add such tests. So we extend the features lint of the tidy tool to ensure that all new lang features contain a new compile-fail test. Of course, one could drop this requirement and just grep all tests in run-pass for #![feature(abc)] and then run this test again, removing the mention, requiring that it fails. But this only tests for the existence of a compilation failure. Manual tests ensure that also the correct lines spawn the error, and also test the actual error message. For library features, it makes no sense to require such a test, as here code is used that is generic for all library features.

… feature $f

Now, no feature outside of the whitelist is without a test marked as its gate test.

Rebase on top of PR 38814 made required this.

est31 · 2017-01-12T22:56:44Z

As #38814 got merged, I had to rebase and mark the test it added. re-r? @nikomatsakis

nikomatsakis · 2017-01-13T17:42:38Z

@bors r+

bors · 2017-01-13T17:42:39Z

📌 Commit c6f99b4 has been approved by nikomatsakis

bors · 2017-01-14T06:42:34Z

⌛ Testing commit c6f99b4 with merge 6fe2371...

@nikomatsakis

Make tidy check for lang gate tests Add gate tests to the checks that tidy performs. Excerpt from the commit message of the main commit: Require compile-fail tests for new lang features Its non trivial to test lang feature gates, and people forget to add such tests. So we extend the features lint of the tidy tool to ensure that all new lang features contain a new compile-fail test. Of course, one could drop this requirement and just grep all tests in run-pass for #![feature(abc)] and then run this test again, removing the mention, requiring that it fails. But this only tests for the existence of a compilation failure. Manual tests ensure that also the correct lines spawn the error, and also test the actual error message. For library features, it makes no sense to require such a test, as here code is used that is generic for all library features. The tidy lint extension now checks the compile-fail test suite for occurences of "gate-test-X" where X is a feature. Alternatively, it also accepts file names with the form "feature-gate-X.rs". If a lang feature is found that has no such check, we emit a tidy error. I've applied the markings to all tests I could find in the test suite. I left a small (20 elements) whitelist of features that right now have no gate test, or where I couldn't find one. Once this PR gets merged, I'd like to close issue #22820 and open a new one on suggestion of @nikomatsakis to track the removal of all elements from that whitelist (already have a draft). Writing such a small test can be a good opportunity for a first contribution, so I won't touch it (let others have the fun xD). cc @brson , @pnkfelix (they both discussed about this in the issue linked above).

bors · 2017-01-14T08:47:50Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 6fe2371 to master...

rust-highfive assigned nikomatsakis Jan 8, 2017

nikomatsakis approved these changes Jan 10, 2017

View reviewed changes

nikomatsakis reviewed Jan 10, 2017

View reviewed changes

nikomatsakis requested changes Jan 10, 2017

View reviewed changes

est31 force-pushed the tidy-gate-tests branch 2 times, most recently from 14fab21 to 66718ed Compare January 11, 2017 01:49

nikomatsakis reviewed Jan 11, 2017

View reviewed changes

nikomatsakis approved these changes Jan 11, 2017

View reviewed changes

est31 added 8 commits January 12, 2017 23:53

Use only one feature struct, and use hash maps in both occasions

e8b6d3c

Add gate-test- to some already existing tests

0bcd3b2

Don't require gate tests for removed features

6795ddf

Automatically treat test files "feature-gate-$f.rs" as gate tests for…

3f6964e

… feature $f

Use a whitelist and print nice hints

4045729

Mark even more tests as gate tests

6a29fab

Now, no feature outside of the whitelist is without a test marked as its gate test.

Add docs

bdee52e

Also mark the struct_field_attributes regression test

c6f99b4

Rebase on top of PR 38814 made required this.

est31 force-pushed the tidy-gate-tests branch from 66718ed to c6f99b4 Compare January 12, 2017 22:55

bors merged commit c6f99b4 into rust-lang:master Jan 14, 2017

This was referenced Jan 14, 2017

Removal of the lang feature gate tests whitelist #39059

Closed

exclusive range patterns #35712

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make tidy check for lang gate tests #38914

Make tidy check for lang gate tests #38914

est31 commented Jan 8, 2017 •

edited

Loading

rust-highfive commented Jan 8, 2017

nikomatsakis Jan 10, 2017

est31 Jan 10, 2017

nikomatsakis commented Jan 10, 2017

bors commented Jan 10, 2017

nikomatsakis commented Jan 10, 2017

nikomatsakis commented Jan 10, 2017

nikomatsakis Jan 10, 2017

est31 Jan 10, 2017

nikomatsakis left a comment

est31 commented Jan 10, 2017 •

edited

Loading

nikomatsakis commented Jan 10, 2017

est31 commented Jan 11, 2017

est31 commented Jan 11, 2017

est31 commented Jan 11, 2017

est31 commented Jan 11, 2017

nikomatsakis Jan 11, 2017

nikomatsakis commented Jan 11, 2017

bors commented Jan 11, 2017

nikomatsakis commented Jan 11, 2017

est31 commented Jan 12, 2017

nikomatsakis commented Jan 13, 2017

bors commented Jan 13, 2017

bors commented Jan 14, 2017

bors commented Jan 14, 2017

Make tidy check for lang gate tests #38914

Make tidy check for lang gate tests #38914

Conversation

est31 commented Jan 8, 2017 • edited Loading

rust-highfive commented Jan 8, 2017

nikomatsakis Jan 10, 2017

Choose a reason for hiding this comment

est31 Jan 10, 2017

Choose a reason for hiding this comment

nikomatsakis commented Jan 10, 2017

bors commented Jan 10, 2017

nikomatsakis commented Jan 10, 2017

nikomatsakis commented Jan 10, 2017

nikomatsakis Jan 10, 2017

Choose a reason for hiding this comment

est31 Jan 10, 2017

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

est31 commented Jan 10, 2017 • edited Loading

nikomatsakis commented Jan 10, 2017

est31 commented Jan 11, 2017

est31 commented Jan 11, 2017

est31 commented Jan 11, 2017

est31 commented Jan 11, 2017

nikomatsakis Jan 11, 2017

Choose a reason for hiding this comment

nikomatsakis commented Jan 11, 2017

bors commented Jan 11, 2017

nikomatsakis commented Jan 11, 2017

est31 commented Jan 12, 2017

nikomatsakis commented Jan 13, 2017

bors commented Jan 13, 2017

bors commented Jan 14, 2017

bors commented Jan 14, 2017

est31 commented Jan 8, 2017 •

edited

Loading

est31 commented Jan 10, 2017 •

edited

Loading