Add random testing of crates.io regex #472

ethanpailes · 2018-04-29T17:42:00Z

This PR implements the first half of #468.

I've marked it as a WIP until I get to the bottom of the \b find_iter bug.

The tests this PR adds take a long time to run, so I disabled them by default, but added logic to ci/script.sh to run them during a travis.ci cron job. I think someone with access to the travis console will need to enable that (so probably @BurntSushi or @alexcrichton). I'm also totally happy to rip the ci changes out or just leave the tests always enabled.

Commit Msg:

This patch adds some infastructure to scrape crates.io
for regex, then run each of the regex found in this way though
a random testing gauntlet to make sure that all the different
backends behave in the same way. These random tests are
expensive, so we add a feature gate around them so that
normal CI does not get bogged down. If the CI tests are being
run in a cron job, we trigger the random testing.

To make this better we should add something which can generate
a matching string from a regex. As is this will just focus on
the negative case.

BurntSushi

Thanks for working on this! I left a number of comments, and most of them are just nits. My higher level thoughts here:

Running these tests in the cron but not in normal builds sounds like a nice compromise. CI is already setup to run a daily cron.
It might speed up tests if we enable compiler optimizations for tests. This will make compilation longer, which is why this isn't enabled today. But it might help quite a bit with the quickcheck tests. I'm not quite sure how to selectively enable this just for the consistency tests though (other than a sed command that modifies the Cargo.toml in place).

Also, could you give a rough time for how long it takes to run these tests?

BurntSushi · 2018-04-29T19:48:03Z

Cargo.toml

@@ -57,6 +57,9 @@ pattern = []
 # Enable to use simd acceleration.
 # Note that this is deprecated and is a no-op.
 simd-accel = []
+# When testing, run the expensive randomized testing suites in addition
+# to the fast unit tests.
+random-test = []


I think we probably don't want a feature for this. Features are part of a crate's public API, and this is really an internal implementation detail of a specific kind of test.

We can instead use normal cfg's, which might be set by env vars. We already do this for enabling simd vector optimizations in the build.rs.

Good point. I've switched the code to use an environment variable. I wasn't sure how to do that through the cfg system so I used the option_env! macro instead.

BurntSushi · 2018-04-29T19:48:41Z

scripts/scrape_crates_io.py

+import argparse
+import datetime
+import time
+from subprocess import call


Can you sort imports alphabetically? Thanks.

You probably want to run a pep8 checker on this file. I see some violations.

TIL pycodestyle is a thing. Thanks!

BurntSushi · 2018-04-29T19:50:12Z

tests/consistent.rs

@@ -0,0 +1,213 @@
+extern crate regex;
+extern crate quickcheck;


I generally try to keep extern crate statements in the crate root, even if this means repeating some cfgs.

Should be fixed in the patch I just pushed.

BurntSushi · 2018-04-29T19:51:11Z

tests/consistent.rs

+
+macro_rules! checker {
+    ($module_name:ident, $regex_type:path, $mk_input:expr) => {
+        mod $module_name {


I'd recommend out-denting the module declaration, and then indenting its contents from there.

BurntSushi · 2018-04-29T19:53:14Z

tests/test_crates_regex.rs

+//                                                                   //
+//                       The Bug Trophy Case                         //
+//                                                                   //
+///////////////////////////////////////////////////////////////////////


This style of comment isn't anywhere else in the regex crate, and I'm generally not a fan of them. They take up a lot of space, and I'm not really sure what they mean. My guess is that they are just regression tests, and we should probably call them that.

I'll delete this section once I drill down on that word boundary bug.

BurntSushi · 2018-04-29T19:54:50Z

ci/script.sh

 fi
+cargo test --verbose ${CARGO_TEST_ARGS}


Why did the else block get removed here? I guess it's probably OK, but this will cause the nightly CI test to get quite a bit longer.

Well my plan was to just build up an arg string in the if statements so that random testing was handled seperatly from the unstable feature checking, then execute a single cargo test will all the args, but now I realize that this would cause two --features flags to be passed :(. This is going away as part of replacing the feature with an env var anyway.

BurntSushi · 2018-04-29T19:58:44Z

tests/consistent.rs

+    }
+}
+
+impl quickcheck::Testable for RegexEqualityTest {


This is kind of weird? What made you implement Testable explicitly here?

I ran into some trouble with the borrow checker when I tried to use a closure over the two regex in question. I think the closures tried to take ownership of the regex, when I wanted to copy them instead.

ethanpailes · 2018-04-30T00:47:16Z

Thanks for the review!

I tried to address the comments in the patch I just pushed.

This PR is still blocked on the word boundary bug, I'll try to get to that soon.

WRT time it takes about 35 seconds in release mode and 9ish minutes in debug mode. Wow that makes a difference! I think I should probably just ditch the ci shenannigans and enable release mode for the random testing every time.

BurntSushi · 2018-04-30T00:49:54Z

I think I should probably just ditch the ci shenannigans and enable release mode for the random testing every time.

Sounds good!

This PR is still blocked on the word boundary bug, I'll try to get to that soon.

If this turns out to be hard (and it may very well be), would it be possible to comment out those tests for now in the interest of landing the PR? I think this is overall great work, and I don't want to put you in a position where you need to fix existing bugs before landing it.

ethanpailes · 2018-04-30T00:55:53Z

If this turns out to be hard (and it may very well be), would it be possible to comment out those tests for now in the interest of landing the PR? I think this is overall great work, and I don't want to put you in a position where you need to fix existing bugs before landing it.

Definitly! The last bug turned out the be pretty shallow when I actually looked at it, so I'm hoping this will be the same. If it leads to lazy DFA arcana I won't hesitate to bail out.

Do you happen to know if there is an easy way to enable release mode for just one test binary? If there isn't I can probably break out the build.rs hackery, but that seems gross to me.

BurntSushi · 2018-04-30T10:48:52Z

Not to my knowledge unfortunately. build.rs hackery sounds like the best bet.

ethanpailes · 2018-04-30T16:53:09Z

I had trouble with the word boundary bug, so I commented it out. I've removed the WIP status.

I also had trouble figuring out a build.rs hack because it looks like you are not allowed to specify arbitrary rustc flags. Instead I went back to the environment variable and CI script approach. The difference is that I no longer special case cron jobs. The random tests are run in release mode on every CI run.

ethanpailes · 2018-06-21T14:09:47Z

I rebased this patch on top of master. Github is telling me there are changes requested, but I think the patches those comments were attached to got lost to rebasing. I do remember taking steps to address all of them though. If there is anything else I can do, please let me know!

BurntSushi

Thanks for the ping!

So I guess my concern here is how long it takes to not just run these tests, but compile them as well. In my comments above, I mentioned build.rs trickery. The purpose of build.rs trickery is to not only control whether these tests get run but whether these tests get compiled as well.

Before doing anything on this though, could you say how long it takes to compile and run these tests? If it's not that much longer, then we can probably just live with it.

BurntSushi · 2018-06-21T14:26:37Z

ci/script.sh

-  cargo test --verbose --features unstable
-else
-  cargo test --verbose
+  CARGO_TEST_EXTRA_FLAGS="--features unstable"


Incidentally, I think this whole thing can just be removed now. unstable is now a no-op. :-)

I removed the features but left the logic in place in case it is ever used again.

BurntSushi · 2018-06-21T14:28:35Z

tests/consistent.rs

+
+    Ok(try!(string_checker::check_backends(&standard_backends))
+       + try!(string_checker::check_backends(&utf8bytes_backends))
+       + try!(bytes_checker::check_backends(&bytes_backends)))


Probably shouldn't be using try! any more. :-) We now require Rust 1.20!

I've switched from try! to ?.

This patch adds some infastructure to scrape crates.io for regex, then run each of the regex found in this way though a random testing gauntlet to make sure that all the different backends behave in the same way. These random tests are expensive, so we only compile them in when the magic `RUST_REGEX_RANDOM_TEST` environment variable is set. In debug mode, these tests take quite a while, so we special case them in CI to run in release mode. Cargo does not support per-target profiles and `build.rs` does not seem to be allowed to set any rustc flags except for linking flags, so we have to manually do it in the CI script. To make this better we should add something which can generate a matching string from a regex. As is we just focus on the negative case. There is one bug that this uncovered that this patch does not fixed. A minimal version of it is commented out in the `tests/test_crates_regex.rs` file.

ethanpailes · 2018-06-21T15:16:53Z

On my machine it takes 3:36 of wallclock time (though 17:30 of CPU time). Milage may vary on the different CI servers, but I think that the startup time of AppVeyor will probably still dominate.

BurntSushi · 2018-06-21T15:21:15Z

@ethanpailes Hmm. I'm not getting the answer I'm looking for, so I must be asking the wrong question.

Forget about CI for a moment. Adding a couple minutes to an already 10 minute build is probably something I can live with, and we don't need to make it conditional. (It would perhaps be nice to only run them on nightly and skip them on the rest.)

What I care about a lot more is the cycle time during development. There are three different things to test here:

Compile and run all tests.
Compile all tests, run everything except for the crates.io tests.
Do not compile the crates.io tests at all, but compile and run everything else.

This is the kind of thing I do a lot when working on the regex crate, so it's important to keep this cycle time down. I don't know which option to pick though because I don't have the data.

When I have time, I can pull down this PR and figure out the data points if this is still unclear.

ethanpailes · 2018-06-21T15:59:55Z

That makes sense. I'll try to provides some times that outline of various OODA loops with notes on how I personally use each that may or may not be useful.

Action	Time (wallclock)	Notes
`cargo clean && cargo test`	1:49	This one probably does not matter that much, but it is still worth knowing. `cargo test` alone will not actually run the crates.io tests because the RUST_REGEX_RANDOM_TEST environment variable is not set.
After a clean build insert `println!("a change")` into `src/exec.rs:ExecBuilder::new` and `cargo test`	1:14	I run this sort of thing when I'm finalizing a patch. Again, will not run the crates.io tests.
After a clean build insert `println!("a change")` into `src/exec.rs:ExecBuilder::new` and `cargo test --test default`	0:15	This sort of thing is my main driver and probably should not have changed.
After a clean build remove `println!("a change")` from `src/exec.rs:ExecBuilder::new` and `export RUST_REGEX_RANDOM_TEST=1 && time cargo test --release --test crates-regex`	0:48	This will run the crates.io tests, and is faster than compiling in debug mode. Does not run any other tests.
`./ci/script.sh`	3:36	This one compiles and runs all tests, including the crates.io tests. I usually do this when finalizing a patch. The best way to compile and run all commands is with two commands (one for the crates.io tests in release mode, and one for the rest in debug mode).

I don't have a time for (2) from your list, because I don't know of a negative test filter you can pass to cargo test, so the only way I can think to do it is to enumerate all the other tests, which seems annoying.

BurntSushi · 2018-06-21T16:04:54Z

I don't have a time for (2) from your list, because I don't know of a negative test filter you can pass to cargo test, so the only way I can think to do it is to enumerate all the other tests, which seems annoying.

I think it's actually the second entry in your table, no? That is, it compiles all tests, but runs everything except for the crates.io tests.

I think the thing you don't have is how long cargo test takes without needing to compile the crates.io tests. (This is (3).)

With that said, it's a good point about cargo test --test default. That does tend to be the main thing that I run, but I do just run cargo test a bit as well.

ethanpailes · 2018-06-21T16:36:12Z

I think it's actually the second entry in your table, no?

You're right. Derp.

I do just run cargo test a bit as well

1:15 is something I'd personally be comfortable with for an outer feedback cycle (I might have been warped by 15 minute C++ outer feedback cycles), but it is definitly starting to become annoying. I think ideally we would configure cargo test with no extra args not to compile the crates.io tests. I ran out the steam on that last time, but I can take another crack at it if you want.

BurntSushi · 2018-06-21T16:44:41Z

It's fine. Let's just merge this, and if I become annoyed, I'll fix it. :-)

BurntSushi · 2018-06-21T16:45:50Z

Thanks again!

ethanpailes · 2018-06-21T16:54:20Z

Thanks!

BurntSushi requested changes Apr 29, 2018

View reviewed changes

ethanpailes force-pushed the test-crates-io branch from b01cad9 to a7b4543 Compare April 30, 2018 16:43

ethanpailes changed the title ~~[WIP] Add random testing of crates.io regex~~ Add random testing of crates.io regex Apr 30, 2018

ethanpailes force-pushed the test-crates-io branch from a7b4543 to 0d615fb Compare April 30, 2018 16:50

ethanpailes force-pushed the test-crates-io branch 2 times, most recently from d7059c2 to 9f9dd76 Compare June 21, 2018 14:00

BurntSushi reviewed Jun 21, 2018

View reviewed changes

ethanpailes force-pushed the test-crates-io branch from 9f9dd76 to 1b8d016 Compare June 21, 2018 15:15

BurntSushi merged commit 5fd3bd1 into rust-lang:master Jun 21, 2018

ethanpailes deleted the test-crates-io branch June 21, 2018 16:54

BurntSushi mentioned this pull request Mar 6, 2023

Test Regex Scraped from crates.io #468

Closed

		@@ -0,0 +1,213 @@
		extern crate regex;
		extern crate quickcheck;

Add random testing of crates.io regex #472

Add random testing of crates.io regex #472

Conversation

ethanpailes commented Apr 29, 2018

BurntSushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanpailes commented Apr 30, 2018

BurntSushi commented Apr 30, 2018

ethanpailes commented Apr 30, 2018

BurntSushi commented Apr 30, 2018

ethanpailes commented Apr 30, 2018

ethanpailes commented Jun 21, 2018

BurntSushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanpailes commented Jun 21, 2018

BurntSushi commented Jun 21, 2018 • edited Loading

ethanpailes commented Jun 21, 2018 • edited Loading

BurntSushi commented Jun 21, 2018 • edited Loading

ethanpailes commented Jun 21, 2018

BurntSushi commented Jun 21, 2018

BurntSushi commented Jun 21, 2018

ethanpailes commented Jun 21, 2018

BurntSushi commented Jun 21, 2018 •

edited

Loading

ethanpailes commented Jun 21, 2018 •

edited

Loading

BurntSushi commented Jun 21, 2018 •

edited

Loading