Feat/property tests from generate #27

shnewto · 2017-11-12T05:13:31Z

Revised generate test.

Prior to this commit the generate tests only generated one
random grammar to parse per run. This commit revises the approach
and instead uses the generate function as an arbitrary property
to test the the Grammar::from_str functionality with.

Additions relevant to the reopening of this PR:

First, a new error kind was introduced to indicate that
the generation error isn't generic but the result of the
limits of recursion being reached. This allows the
property tests for the Grammar type to disregard problematic
sentence generation, it is testing that what it gets is parsable.

In order to manage meeting recursion limits being met in the
property tests, a new use case was accounted for, one that I
belive implementaion for is useful. Logic was added to allow the
"from_str" trait for bnf types to be parallel to Type::new()
in all cases except the Term type. Because the Term type does
not have a sensical way to implement new(), "from_str" for Term
types corresponds to a Terminal rule for the empty string
which is allowed in parsable gramamrs.

As this commit represents that addition of a new error kind
it seemed that there is enough variety now to make our Error
type public in order for users can match on them.

Prior to this commit the generate tests only generated one random grammar to parse per run. This commit revises the approach and instead uses the generate function as an arbitrary property to test the the Grammar::from_str functionality with.

coveralls · 2017-11-12T05:22:27Z

Coverage increased (+0.05%) to 93.758% when pulling d58dec2 on feat/property-tests-from-generate into 69d84d6 on master.

…or tests

CrockAgile · 2017-11-12T05:44:58Z

tests/grammar.rs

-        let meta_grammar = bnf::Grammar::from_str(&sentence.unwrap());
-        assert!(meta_grammar.is_ok());
+    impl Arbitrary for Meta {
+        fn arbitrary<G: Gen>(_: &mut G) -> Meta {


According to the quickcheck docs on its Arbitrary trait the provided Gen should be used to generate the arbitrary test data. It also highlights the importance of being able to shrink the data to a minimal failing case. This implementation seems to violate both of these contraints

ahh, that's interesting, like maybe use that to seed random?

Yeah we really should be able to reproduce any failures. I'll look into using an approach that lets us provide the seed

Seems like it. And that kind of control of generate's random isn't public to integration tests like this. Do we make it public via another API? Do we move this property testing within the Grammar module? Is it okay to ignore the "property" aspect of property based testing and only use it as a test runner than executes multiple times?

Shrinking a Grammar seems involved. Seems like would have to keep the grammar in its full tree state and do some branch trimming/grafting.

Also, does the meta BNF grammar guarantee non-infinite grammars? Because otherwise this test occasionally fails when the generated grammar is unlucky enough to blow the stack

I think if we're seeding then we only have to shrink on the seed. Like it's enough to reproduce to know if you seed with xyz you generate a fail case

It does seem like I'd taken for granted a step that should have property tests applied to it

Maybe shrinking is overly complex for BNF and these tests just opt out? Once a failure, shrinking does nothing. As I read the quickcheck docs, that seems to be the default behavior, so maybe it is not important. The key point seems to be that the same Gen needs to give the same test case.

coveralls · 2017-11-12T05:45:32Z

Coverage decreased (-0.09%) to 93.625% when pulling fc7c34b on feat/property-tests-from-generate into 69d84d6 on master.

CrockAgile · 2017-11-12T05:49:29Z

tests/grammar.rs

@@ -19,7 +19,7 @@ mod tests {
            <rule>           ::= <opt-whitespace> \"<\" <rule-name> \">\"
                                <opt-whitespace> \"::=\" <opt-whitespace>
                                <expression> <line-end>
-            <opt-whitespace> ::= \" \" <opt-whitespace> | \"\"
+            <opt-whitespace> ::=  \"\" | \" \" <opt-whitespace>


I must be missing something. Does this just swap the expression order?

coveralls · 2017-11-12T05:53:22Z

Coverage increased (+0.05%) to 93.766% when pulling ca08e5d on feat/property-tests-from-generate into 69d84d6 on master.

CrockAgile · 2017-11-12T06:00:14Z

tests/grammar.rs

@@ -19,7 +19,7 @@ mod tests {
            <rule>           ::= <opt-whitespace> \"<\" <rule-name> \">\"
                                <opt-whitespace> \"::=\" <opt-whitespace>
                                <expression> <line-end>
-            <opt-whitespace> ::=  \"\" | \" \" <opt-whitespace>
+            <opt-whitespace> ::=  \"\" | \" \" 


Does this change the meta BNF grammar to only generate grammars with 0 or 1 space between the <rule-name> and ::= ? I thought it was legal to have more than 1.

Yeah it does, I'm not sure it's something we should remove if we don't have to, it was just the likely candidate for a lot of recursion and I wanted to see if things all built without it

shnewto · 2017-11-12T06:09:47Z

Some revision needs to happen to allow Grammar::generate to expose the functionality required for reproducible tests.

coveralls · 2017-11-12T07:41:44Z

Coverage increased (+0.1%) to 93.82% when pulling e180c01 on feat/property-tests-from-generate into 69d84d6 on master.

coveralls · 2017-11-12T07:41:44Z

Coverage increased (+0.1%) to 93.82% when pulling e180c01 on feat/property-tests-from-generate into 69d84d6 on master.

coveralls · 2017-11-12T07:41:44Z

Coverage increased (+0.1%) to 93.82% when pulling e180c01 on feat/property-tests-from-generate into 69d84d6 on master.

Changes demand new review

CrockAgile · 2017-11-12T07:53:49Z

src/grammar.rs

@@ -92,8 +92,8 @@ impl Grammar {

        let expression;
        let expressions = production.rhs_iter().collect::<Vec<&Expression>>();
-
-        match thread_rng().choose(&expressions) {
+


is github lying about this whitespace?

CrockAgile · 2017-11-12T07:58:04Z

src/grammar.rs

@@ -104,7 +104,7 @@ impl Grammar {

        let mut result = String::new();
        for term in expression.terms_iter() {
-            match self.eval_terminal(&term) {
+            match self.eval_terminal(&term, rng) {


Does this mean each term gets its own copy of the StdRng ? Seems strange for each term to essentially be given the same seed, or does it do something else?

CrockAgile · 2017-11-12T08:00:11Z

src/grammar.rs

@@ -137,7 +142,7 @@ impl Grammar {
    ///     # assert!(sentence_clone.is_ok());
    /// }
    /// ```
-    pub fn generate(&self) -> Result<String, Error> {
+    pub fn generate_seeded(&self, rng: StdRng) -> Result<String, Error> {


Related to my previous comment, does this have to take ownership of the StdRng or could it work with a mutable reference?

Prior to this commit the BNF for BNF had a rule that catered to deep recursion without, to me, adding any real value as sensical syntax despite that it is how the wikipedia source defines the grammar. Although the revision to the gramamr makes our failure case much less likely, I also added some logic to allow the stack to grow for deep recursion in order to give grammars that might require it more room to generate. The fact that ours had failed with regularity made me think there is a use case for allowing more stack within a threshold. That threshold is exercised by our `infinite_loop` test.

…nd use quickcheck default

coveralls · 2017-11-12T18:56:09Z

Coverage increased (+0.2%) to 93.865% when pulling 762db35 on feat/property-tests-from-generate into 69d84d6 on master.

coveralls · 2017-11-12T19:03:36Z

Coverage increased (+0.2%) to 93.865% when pulling 524a334 on feat/property-tests-from-generate into 69d84d6 on master.

CrockAgile · 2017-11-12T19:32:37Z

Is your rustfmt still working? Or are we somehow on major different versions? Because when I pulled this down to play with it, my editor made a lot of auto changes. I'm on 0.9.0-nightly rustfmt

shnewto · 2017-11-12T19:36:28Z

ha no the VS code started crashing my computer so I disabled it. That's a bummer about formatting, I can manually run rustfmt on it all tho

coveralls · 2017-11-12T19:52:56Z

Coverage decreased (-0.1%) to 93.58% when pulling cdb20f0 on feat/property-tests-from-generate into 69d84d6 on master.

coveralls · 2017-11-12T20:09:39Z

Coverage decreased (-0.2%) to 93.525% when pulling 0199933 on feat/property-tests-from-generate into 69d84d6 on master.

shnewto · 2017-11-13T17:03:14Z

I'd like to revise the grammar property tests to handle the excessive recursion potential as well as factor out the logic to potentially grow the stack to account for that heavy recursion.

This commit contains a few revisions. First, a new error kind was introduced to indicate that the generation error isn't generic but the result of the limits of recursion being reached. This allows the property tests for the Grammar type to disregard problematic sentence generation, it is testing that what it gets is parsable. In order to manage meeting recursion limits being met in the property tests, a new use case was accounted for, one that I belive implementaion for is useful. Logic was added to allow the "from_str" trait for bnf types to be parallel to Type::new() in all cases except the Term type. Because the Term type does not have a sensical way to implement new(), "from_str" for Term types corresponds to a Terminal rule for the empty string which is allowed in parsable gramamrs. As this commit represents that addition of a new error kind it seemed that there is enough variety now to make our Error type public in order for users can match on them.

coveralls · 2017-11-14T19:57:23Z

Coverage decreased (-0.7%) to 92.993% when pulling 75d68fc on feat/property-tests-from-generate into 98dc357 on master.

coveralls · 2017-11-14T19:59:53Z

Coverage decreased (-0.7%) to 92.993% when pulling 75d68fc on feat/property-tests-from-generate into 98dc357 on master.

coveralls · 2017-11-14T20:14:37Z

Coverage decreased (-0.2%) to 93.473% when pulling b19a530 on feat/property-tests-from-generate into 98dc357 on master.

CrockAgile · 2017-11-14T20:19:53Z

src/expression.rs

@@ -25,6 +25,9 @@ impl Expression {

    // Get `Expression` by parsing a string
    pub fn from_str(s: &str) -> Result<Self, Error> {
+        if s.len() == 0 {
+            return Ok(Expression::new());
+        }


Does the expression parser not handle end of input? This special case separation between parser and from_str behavior seems strange.

CrockAgile · 2017-11-14T20:21:57Z

src/grammar.rs

+        // match sentence.unwrap() {
+        //     Error::RecursionLimit(_) => (),
+        //     e => panic!("should should be Error::RecursionLimit: {:?}", e),
+        // }


Vestigial code?

CrockAgile · 2017-11-14T20:23:44Z

src/term.rs

@@ -14,6 +14,12 @@ pub enum Term {
 impl Term {
    // Get `Term` by parsing a string
    pub fn from_str(s: &str) -> Result<Self, Error> {
+        // this is a little weird, should we default to Nonterminal?
+        // Seems like empty would correspond to a terminal that represents
+        // the empty string.


I agree with this comment. Not sure if you meant this as an actual question for me to be removed later or if the doubt is something you would like to keep in comments

coveralls · 2017-11-14T21:36:24Z

Coverage decreased (-1.5%) to 92.177% when pulling 9ec5a8d on feat/property-tests-from-generate into 98dc357 on master.

coveralls · 2017-11-14T22:06:46Z

Coverage decreased (-1.5%) to 92.177% when pulling 608f666 on feat/property-tests-from-generate into 98dc357 on master.

coveralls · 2017-11-14T22:19:00Z

Coverage decreased (-1.5%) to 92.177% when pulling 89b23c9 on feat/property-tests-from-generate into 98dc357 on master.

Feat/property tests from generate

shnewto added 3 commits November 11, 2017 21:06

Revised generate test

e7ef0d4

Prior to this commit the generate tests only generated one random grammar to parse per run. This commit revises the approach and instead uses the generate function as an arbitrary property to test the the Grammar::from_str functionality with.

add some error report should the assert on sentence fails

092bf0e

eof newline

d58dec2

shnewto requested a review from CrockAgile November 12, 2017 05:13

looks like there was a non-terminating rule in the bnf for bnf used f…

fc7c34b

…or tests

CrockAgile previously requested changes Nov 12, 2017

View reviewed changes

don't allow unlimited optional whitespace

ca08e5d

CrockAgile reviewed Nov 12, 2017

View reviewed changes

shnewto closed this Nov 12, 2017

shnewto added 3 commits November 11, 2017 23:03

filling in option to seed generate for reproducibility

159f3ab

filling in the seedable logic

5ca6db4

updating tests to use seeded generate

e180c01

shnewto reopened this Nov 12, 2017

CrockAgile requested changes Nov 12, 2017

View reviewed changes

shnewto added 2 commits November 12, 2017 10:38

remove experimentation for defining how many tests to run / succeed a…

524a334

…nd use quickcheck default

CrockAgile approved these changes Nov 12, 2017

View reviewed changes

pass all the source through rustfmt

cdb20f0

add incomplete tests to increase coverage this branch had decreased

0199933

shnewto closed this Nov 13, 2017

shnewto added 4 commits November 14, 2017 11:05

updating to the state and testing conventions on the master branch

f2c3a0e

run source through rustfmt

e6f86cb

adding test to increase coverage report

75d68fc

shnewto reopened this Nov 14, 2017

shnewto closed this Nov 14, 2017

shnewto reopened this Nov 14, 2017

shnewto requested a review from CrockAgile November 14, 2017 19:56

filling in tests on uncovered LOC

b19a530

CrockAgile reviewed Nov 14, 2017

View reviewed changes

revert success on empty string

9ec5a8d

shnewto added 2 commits November 14, 2017 13:47

attempt to keep test LOC from test coverage report

0f1c723

experimenting with cargo coveralls options

608f666

revert travis file and set fail threshold in coveralls settings

89b23c9

shnewto merged commit 508e7f8 into master Nov 14, 2017

shnewto deleted the feat/property-tests-from-generate branch November 14, 2017 22:22

shnewto added a commit that referenced this pull request Jul 29, 2020

Merge pull request #27 from snewt/feat/property-tests-from-generate

7a97114

Feat/property tests from generate

Feat/property tests from generate #27

Feat/property tests from generate #27

Conversation

shnewto commented Nov 12, 2017 • edited

coveralls commented Nov 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnewto Nov 12, 2017 • edited

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

coveralls commented Nov 12, 2017 • edited

Choose a reason for hiding this comment

coveralls commented Nov 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shnewto commented Nov 12, 2017

coveralls commented Nov 12, 2017

coveralls commented Nov 12, 2017

coveralls commented Nov 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 12, 2017 • edited

coveralls commented Nov 12, 2017 • edited

CrockAgile commented Nov 12, 2017

shnewto commented Nov 12, 2017 • edited

coveralls commented Nov 12, 2017 • edited

coveralls commented Nov 12, 2017 • edited

shnewto commented Nov 13, 2017

coveralls commented Nov 14, 2017 • edited

coveralls commented Nov 14, 2017 • edited

coveralls commented Nov 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 14, 2017 • edited

coveralls commented Nov 14, 2017 • edited

coveralls commented Nov 14, 2017 • edited

shnewto commented Nov 12, 2017 •

edited

coveralls commented Nov 12, 2017 •

edited

shnewto Nov 12, 2017 •

edited

coveralls commented Nov 12, 2017 •

edited

coveralls commented Nov 12, 2017 •

edited

coveralls commented Nov 12, 2017 •

edited

coveralls commented Nov 12, 2017 •

edited

shnewto commented Nov 12, 2017 •

edited

coveralls commented Nov 12, 2017 •

edited

coveralls commented Nov 12, 2017 •

edited

coveralls commented Nov 14, 2017 •

edited

coveralls commented Nov 14, 2017 •

edited

coveralls commented Nov 14, 2017 •

edited

coveralls commented Nov 14, 2017 •

edited

coveralls commented Nov 14, 2017 •

edited

coveralls commented Nov 14, 2017 •

edited