search

ligos / readablepassphrasegenerator

The Readable Passphrase Generator generates passphrases which are (mostly) grammatically correct but nonsensical. These are easy to remember (for humans) but difficult to guess (for humans and computers).
Apache License 2.0
153 stars 11 forks source link

Fixes #3 - ArgumentOutOfRangeException in ConstantMutator #4

Closed AGBrown closed 4 years ago

AGBrown commented 4 years ago

This should fix #3 - the ArgumentOutOfRangeException in ConstantMutator

I've created a unit test project and added a test fixture for the ConstantMutator and submitted a fix for the same.

I've had to make some guesses about how ConstantMutator is meant to behave. I've assumed

  1. The input StringBuilder may or may not have a trailing space depending on what mutators have done before this one is invoked, e.g. "foo" → ". foo"
  2. If the mutation is to be made at the start, or middle of the phrase then the mutation is inserted with a trailing whitespace separator, e.g. "foo bar fizz bang" → "foo . bar fizz bang".
  3. If the mutation is to be made at the end of the phrase then the mutation is inserted without a trailing whitespace separator, and before any whitespace that exists, e.g. "foo 6" → "foo 6." and "foo " → "foo. "

Assumption (3) in particular seems like it might be wrong, so needs reviewing, but is easy enough to change the tests and code if it needs correcting.

The penultimate commit (4e8f38b) will fail the relevant test, and the final commit (bf246f4) makes the tests pass by fixing the Mutate method. The rest of the commits are just setting up the test infrastructure as there wasn't a unit test project already in place (unless I missed it).

AGBrown commented 4 years ago

I just had an intermittent test failure on Mutate_ForAnywhere_AddsMutation_BetweenWords. I'm not sure why, so I'll do test runs until failure again to see if it's an issue with the test fixtures or something else.

AGBrown commented 4 years ago

It's an issue with the encoding of the test words file scowl wordlist up to 50.txt in the RandomiseWordsList project. I'll look at a fix for it later/tomorrow.

ligos commented 4 years ago

Thanks @AGBrown - I'm starting work now, but I'll check your changes in more detail tonight.

AGBrown commented 4 years ago

Encoding issue fixed with 140cf3b. The file was encoded as windows 1252 after my git clone (according to sublime, which seemed to read it all correctly as compared to the tests which were reading many characters in as ). As the code I grabbed from RandomiseWordsList used File.OpenText it was using UTF8 encoding, therefore the accented characters were read as U+FFFC which failed Char.IsLetterOrDigit and so the word boundaries were incorrect compared to what was expected by the tests.

I added the nugets to the test project, updated the StreamReader to use the right encoding and now it reads the accented characters correctly. A 10 minute "run unit tests until fail" run didn't fail so I'm as sure as I can be that this must have been the cause of the error.

TL;DR: the failure didn't really related to the test itself but this was the simplest way to fix and move on for the moment.

ligos commented 4 years ago

Thanks for your work, Andrew. I won't merge this pull request (as appealing as having more than zero unit tests is, I doubt I'll have time to improve coverage). I've responded to the original issue.

ligos commented 4 years ago

PS: you were right about assumption 3 - the EndOfWord is a special case to make it look more "natural" without whitespace preceding the .. So I am a sentence. rather than I am a sentence .

ligos commented 4 years ago

Closing as fix was applied via other means.