Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add some reduction methods to the options on the fuzz tests #930

Merged
merged 89 commits into from
Dec 18, 2023

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Oct 7, 2023

This adds a round trip test for config file generation to the fuzzer.

(the next step after this PR will be a fuzzer that verifies that the round trip actually matches the results.
This change ended up requiring quite a few minor changes to fix the ambiguities between the config file generation and config file reader.

1). There was a number of potential conflicts between positional names and regular option names that could be triggered in config files, this required a number of additional checks on the positional naming to ensure no conflicts.
2). flag options with disable flag override can produce output results that are not valid by themselves, resolving this required flag input to be able to handle an array and output the original value set of results.
3). strings with non-printable characters could cause all sorts of chaos in the config files. This was resolved by generating a binary string conversion format and handling multiline comments and characters, and handling escaped characters. Note; I think a better solution is to move to fully supporting string formatting and escaping along with the binary strings from toml now that toml 1.0 is finalized. That will not be this PR though, maybe the next one.
4). Lot of ambiguities and edge cases in the string splitter, this was reworked
5). handling of comments was not done well, especially comments in the name of the option which is allowed.
6). non printable characters in the option naming. This would be wierd in practice but it also cause some big holes in the config file generation, so the restricted character set for option naming was expanded. (don't allow spaces or control characters).

@codecov
Copy link

codecov bot commented Oct 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f043d0c) 100.00% compared to head (a014cb3) 100.00%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##              main      #930    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           18        18            
  Lines         4175      4392   +217     
==========================================
+ Hits          4175      4392   +217     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phlptp
Copy link
Collaborator Author

phlptp commented Oct 11, 2023

There are some inconsistencies that adding this part of the fuzzing exposed in the config file generation. Going to need to support multiline toml strings in the config file generator and reader. Otherwise there is the potential for peculiarly formed strings to generate a config file that is read in a very strange fashion.

@phlptp phlptp mentioned this pull request Oct 15, 2023
@phlptp
Copy link
Collaborator Author

phlptp commented Nov 29, 2023

This is ending up to be quite the adventure. I added fuzzing to generate and read a config file. Turns out there were lots of problems there, taking a while to fix.

@phlptp phlptp requested a review from henryiii December 1, 2023 19:19
@phlptp
Copy link
Collaborator Author

phlptp commented Dec 15, 2023

@henryiii I am going to merge this on 12/18, unless you have some feedback.

@phlptp phlptp merged commit 0f5bf21 into CLIUtils:main Dec 18, 2023
50 checks passed
@phlptp phlptp deleted the fuzz_reduction branch December 18, 2023 13:21
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