Allow QC_TESTS and QC_MAX_TESTS to set tests / max_tests #148
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In practice I've wanted two kinds of quickcheck runs: fast and comprehensive. To date the comprehensive testing has been done by manually adjusting tests / max_tests up from the defaults of 100 / 10000. This involves a recompilation step. If quickcheck were able to read environment variables to set its tests / max_tests that recompilation step would not be needed and a nightly CI task could be written to do a comprehensive QC run. This commit introduces QC_TESTS and QC_MAX_TESTS to meet the above. In the event that QC_MAX_TESTS < QC_TESTS then the max_tests will be set to QC_TESTS. Signed-off-by: Brian L. Troutwine <[email protected]>
blt
pushed a commit
to postmates/cernan
that referenced
this pull request
Oct 21, 2016
This commit adjusts the 'within' determination to avoid the previously identified overlapping interval issue. Happily, the fix is both simpler to understand and a touch faster. We now divide every timestamp by the bin width and take the floor. Boom. This commit also includes code to read QC test adjustments from environment variables. I've submitted a patch upstream to make this convenient for all our QC tests: BurntSushi/quickcheck#148 Signed-off-by: Brian L. Troutwine <[email protected]>
BurntSushi
reviewed
Oct 27, 2016
src/tester.rs
Outdated
|
|
||
| fn qc_tests() -> usize { | ||
| match env::var("QC_TESTS") { | ||
| Ok(val) => val.parse().expect("QC_TESTS value could not converted to number"), |
Owner
There was a problem hiding this comment.
If the parse fails, we definitely shouldn't panic here. I think we should just return the default value if you can't parse a number. (Alternatively, we could make QuickCheck::new return a result, but I think just returning the default on a parse failure is fine.)
Similarly for qc_max_tests.
Owner
|
This is a good idea. I have one nit and one question: do we want to use |
Contributor
Author
|
Cool. I'll fix the panic. Initial motivation was that panicking would give the user a quick indication of a parse failure. Perhaps an error log instead? I can get behind long-form "QUICKCHECK". |
Owner
|
For the most part, if a user sees a panic message, then I tend to consider that a bug. You could add an error log using the |
tsantero
pushed a commit
to postmates/cernan
that referenced
this pull request
Oct 29, 2016
* Remove QOS concept As an experiment the QOS concept for wavefront has failed. We're going, instead, to make the bin size--currently pegged at 1 second-- adjustable for each sink. First step in that work is toasting QOS. Signed-off-by: Brian L. Troutwine <[email protected]> * Implement bin width scale change This commit makes it possible to configure a bucket with a bin-width greater than one, which was the previous hard-coded assumption. The Metric type has grown a 'within' function that allows you to say whether two metrics are within some interval, with the time of the metric called assumed to be in the middle of the interval. Signed-off-by: Brian L. Troutwine <[email protected]> * Make bin_width configurable by sink As of this commit it's possible to configure the bin width for both console and wavefront sinks independently of one another. There remains README updates to document the changes here. Signed-off-by: Brian L. Troutwine <[email protected]> * Update README to reflect new reality The nice thing about ditching the QOS concept is that it's much simpler to explain. Benchmarking shows we got dinged on performance somewhat so we'll need to address that. Signed-off-by: Brian L. Troutwine <[email protected]> * Add tests to demonstrate counterexample This commit adds a shuffling test to more easily demonstrate the overlapping bin failure shown very seldomly by the other sampling test. Signed-off-by: Brian L. Troutwine <[email protected]> * Correct overlapping interval defect This commit adjusts the 'within' determination to avoid the previously identified overlapping interval issue. Happily, the fix is both simpler to understand and a touch faster. We now divide every timestamp by the bin width and take the floor. Boom. This commit also includes code to read QC test adjustments from environment variables. I've submitted a patch upstream to make this convenient for all our QC tests: BurntSushi/quickcheck#148 Signed-off-by: Brian L. Troutwine <[email protected]> * hist -> h
This commit removes the panic when the input value is not able to be parsed and silently replaces whatever goofy value may have been sent with defaults. There is a logging facility that can be exploited to inform the user that a potential mistake has been made but this commit does not explore it. Signed-off-by: Brian L. Troutwine <[email protected]>
In a like fashion to the QUICKCHECK_TEST and QUICKCHECK_MAX_TESTS it's awfully handy to be able to crank the size of the default Gen upward without having to recompile. Signed-off-by: Brian L. Troutwine <[email protected]>
Contributor
Author
|
Hi @BurntSushi, I've incorporated your feedback and included an additional environment variable for controlling |
Owner
|
Fine with me, thanks! :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In practice I've wanted two kinds of quickcheck runs: fast and
comprehensive. To date the comprehensive testing has been done
by manually adjusting tests / max_tests up from the defaults of
100 / 10000. This involves a recompilation step.
If quickcheck were able to read environment variables to set its
tests / max_tests that recompilation step would not be needed and
a nightly CI task could be written to do a comprehensive QC run.
This commit introduces QC_TESTS and QC_MAX_TESTS to meet the
above. In the event that QC_MAX_TESTS < QC_TESTS then the
max_tests will be set to QC_TESTS.
Signed-off-by: Brian L. Troutwine [email protected]