Skip to content

[experiment] Disable all property-based tests#8575

Closed
sholderbach wants to merge 3 commits intonushell:mainfrom
sholderbach:disable-proptests
Closed

[experiment] Disable all property-based tests#8575
sholderbach wants to merge 3 commits intonushell:mainfrom
sholderbach:disable-proptests

Conversation

@sholderbach
Copy link
Copy Markdown
Member

Description

Primarily to study the impact on the coverage I disabled the
property-based tests.
Touches quickcheck and proptest tests (do the same thing slightly
differently under the hood.)
For now just commented out. If we can stop the flukes and keep the
general coverage we would want to move the proptests into a separate
scheduled fuzz run.

User-Facing Changes

None

Tests + Formatting

Yeet the proptests from the earth (or the regularly run CI/coverage tests)

Primarily to study the impact on the coverage I disabled the
property-based tests.
Touches `quickcheck` and `proptest` tests (do the same thing slightly
differently under the hood.)
For now just commented out. If we can stop the flukes and keep the
general coverage we would want to move the proptests into a separate
scheduled fuzz run.
dare you to `-- -D warnings`
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 22, 2023

Codecov Report

Merging #8575 (c90e641) into main (c48e9cd) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8575   +/-   ##
=======================================
  Coverage   68.50%   68.51%           
=======================================
  Files         624      624           
  Lines      100662   100662           
=======================================
+ Hits        68962    68971    +9     
+ Misses      31700    31691    -9     

see 2 files with indirect coverage changes

See if the faster quickcheck test is providing the observed coverage
@sholderbach
Copy link
Copy Markdown
Member Author

Caveat: the indirect coverage tracking aof the parser didn't provide me with a nice diff on codecov.io.
I think the conclusion from the experiment is that the proptests for the nuon format don't seem to cover much more than the now more extensive manual unit tests. I will remove them cleanly, should also shave some seconds of the test runs (and potentially resolve some of the bloat killing the coverage CI jobs #8677 )

What we do with the smaller parser proptest is possibly a bit separate (its removal appeared to show a small drop in coverage)

Longer term goal: move all fuzzing into a separate runner that runs periodically. I am not sure if we can achieve that with feature gating (as the dev-dependencies shouldn't be compiled when not needed) alternatively we need to move that to a separate unpublished crate and only use public API surface,

sholderbach added a commit to sholderbach/nushell that referenced this pull request Mar 31, 2023
The two tests `to_nuon_from_nuon` and `to_nuon_from_nuon_string` were
taking multiple seconds and have since been superseded by more explicit
unit tests. Compared to the time cost for devs and CI they seldomly
returned explicit problems. One failure only popped up after months, as
a sampled failure (nushell#7564).

Fuzzing should move to a separate worker and be removed from the main
test suite.
See nushell#8575 for experimentation around the impact on our test coverage.
sholderbach added a commit that referenced this pull request Mar 31, 2023
# Description

The two tests `to_nuon_from_nuon` and `to_nuon_from_nuon_string` were
taking multiple seconds and have since been superseded by more explicit
unit tests. Compared to the time cost for devs and CI they seldomly
returned explicit problems. One failure only popped up after months, as
a sampled failure (#7564).


# User-Facing Changes

none

# Tests + Formatting

Fuzzing should move to a separate worker and be removed from the main
test suite.
See #8575 for experimentation around the impact on our test coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant