Actually use TestGroupOptions.sizeTests #232
Conversation
Codecov Report
@@ Coverage Diff @@
## master #232 +/- ##
=======================================
Coverage 78.49% 78.49%
=======================================
Files 37 37
Lines 17369 17367 -2
=======================================
- Hits 13633 13632 -1
+ Misses 3736 3735 -1
|
gap/base/recognition.gi
Outdated
| @@ -978,7 +978,7 @@ InstallGlobalFunction( "GetCompositionTreeNode", | |||
|
|
|||
| RECOG.TestGroupOptions := rec( | |||
| # Number of times to test whether the recognized size is right | |||
| sizeTests := 3, | |||
| sizeTests := 1, | |||
There was a problem hiding this comment.
Your PR is named "Actually use TestGroupOptions.sizeTests" but with this change, shouldn't it be named "Actually use TestGroupOptions.sizeTests and change it from 3 to 1"?
Also, why is that change from 3 to 1 desirable?
There was a problem hiding this comment.
Also, why is that change from 3 to 1 desirable?
Because it more accurately reflects what's already happening. While it looks like we currently run the recognition three times, we actually only do it once if it succeeds. Namely, without this PR, when the recognition succeeds for the first time, the count variable is set to three in the line I deleted.
There was a problem hiding this comment.
I've changed the commit message to:
Actually use TestGroupOptions.sizeTests and ..
.. remove the hack in TestGroup which made it look like the recognition
was run thrice.
There was a problem hiding this comment.
OK, I think that's a difference in understanding what sizeTests is good for: I think the idea was to run the test up to sizeTests times, or until it succeeds, i.e, to deal with flaky recognition that misdetects far too often.
So it makes it "easier" to pass the tests.
This PR changes the semantics of this option substantially. The new variant now might be useful to run a test more often, to make it "harder" to pass it -- that's the opposite.
In any case, we never set a custom value for sizeTests, and I don't think we should, so I'd just remove it altogether, and then change this PR to explain that we removed a hack that made it easier to pass certain tests?
There was a problem hiding this comment.
And while I look at it: perhaps we should also change the default for tryNonGroupElements ? It was off by default in the past because it caused lots of failures. But we worked hard on fixing many of them, and really would like to find any remaining ones.
There was a problem hiding this comment.
I think the idea was to run the test up to sizeTests times, or until it succeeds
If the size is incorrect though, it raises an error Error("Alarm: Size not correct!\n") so I think it was used to make tests harder to pass. But yeah, let's get rid of it! :) I opened a PR #234 for the tryNonGroupElements.
.. remove a hack in TestGroup which made it easier to pass it.
No description provided.