test_transcript.py::test_regex_transcript fails if editor includes the letter 's'

[kotfu]

If your setting for editor includes the full path to the editor, i.e. '/usr/local/bin/zile', test_transcript.py::test_regex_transcript fails.

Working on a fix.

[tleonhardt]

@kotfu Thanks for another great catch!

[kotfu]

Deep rabbit hole here. Further testing reveals that test_transcript.py::test_regex_transcript() fails if editor contains an 's'. The path part is a red herring. For example, if editor is 'emacs', the test fails.

The transcript in this test, located in transcript_regex.txt, intends to ensure that that the value of editor does not include any whitespace. cmd2.py::_test_transcript(), reads the transcription file into expected and then does:

expected = self.expectationParser.transformString(expected)

Which strips all backslashes out of expected. That changes the regular expression from ([^\s]+) to ([^s]+).

If the intent of transcript_regex.txt is to check for editor without a space, I don't think you can edit the tile to accomplish that task. The regex needs to include either \s or a space, in order to get the currently desired behavior. The current code in cmd2.py::_test_transcript() removes all whitespace. Trying to escape the backslash in the transcription file doesn't appear to work either. One backslash yields none, two yields none, three yields two, four yields two, five yields two, and six yields two.

expectationParser is set by the following code from cmd2.py:

class Cmd2TestCase(unittest.TestCase):
    """Subclass this, setting CmdApp, to make a unittest.TestCase class
       that will execute the commands in a transcript file and expect the results shown.
       See example.py"""
    cmdapp = None
    regexPattern = pyparsing.QuotedString(quoteChar=r'/', escChar='\\', multiline=True, unquoteResults=True)
    regexPattern.ignore(pyparsing.cStyleComment)
    notRegexPattern = pyparsing.Word(pyparsing.printables)
    notRegexPattern.setParseAction(lambda t: re.escape(t[0]))
    expectationParser = regexPattern | notRegexPattern
    anyWhitespace = re.compile(r'\s', re.DOTALL | re.MULTILINE)

So that's the bottom of the rabbit hole (for now). Let's come up out of the rabbit hole, so I can ask a question. Why is test_transcript.py::test_regex_transcript() trying to make sure editor doesn't contain a space? Even if your EDITOR environment variable contained an editor with a space, this test wouldn't catch it because cmd2.py::_test_transcript() strips all whitespace before applying the transcript. A quick peek at do_edit makes me think if you had a space in your editor, it would fail.

At this point I can think of three ways forward:

1. Fix the pyparsing code in Cmd2TestCase so the \s doesn't get smashed.
2. Change transcript_regex.txt to not care whether editor has a space in it. Not sure if this is advisable, because I don't know why it is attempting (and failing) to ensure there is no space.
3. Fix do_edit so that it works if your filename or editor have spaces in them. Also fix transcript_regex.txt so it doesn't care if editor has spaces.

How do you think I should proceed?

[tleonhardt]

If I recall my thought process, the intent of test_regex_transcript() was two-fold:

1. Help demonstrate to users how they can use a regular expression in a transcript
2. Test that the basic regex feature is working within transcript tests
3. Pass regardless of what editor is set to

A couple things that are probably worth pointing out:

• The transcript testing feature is probably the least "well baked" feature of all features present in cmd2. It works and is useful, but it has some ugly edge cases and warts.
• My regex-fu is really weak. I probably tried to create a regex that made sense for working with "any editor" and settled on the first half-ass thing I Googled that kinda sorta worked ;-)

From my perspective, if the editor contains spaces, that should be OK. If there really is an executable editor with that name and the system can run it in a "subprocess", then I don't see a problem with spaces (that I'm aware of). As you suggest, it doesn't seem like a good idea; but going out of our way to prevent a user from doing something which is a bad idea doesn't necessarily seem like a good idea either.

So out of your options for proceeding, I think I like number 3, fix do_edit and also fix transcript_regex.txt.

What do you think?

[kotfu]

Makes sense. I'll work on:

1. Test and ensure do_edit works if editor has spaces in it. I can't think of any editors that have a space in their executable file name, but I suppose it is possible that it could be a) not in your path, and b) the path where it is located has a space in it.
2. Make sure test_regex_transcript() works whether editor has spaces, esses 😄, or no spaces.

[tleonhardt]

Thank you for your contribution. I appreciate you bringing this issue to our attending and doubly appreciate your help fixing it!

[kotfu]

Just sent a PR to fix. However, it occurs to me that this problem of squashing backslashes in Cmd2TestCase means that any transcript with regular expressions containing backslashes will not behave as expected. Backslashes in regexes are super common, so I'm a little surprised nobody has noticed this, must not be many people using regexes in transcripts.

Should we open another issue to fix Cmd2TestCase so regexes with backslashes work?

[tleonhardt]

This issue was closed by PR #212.

Good catch on all backslashes getting squashed in regexes used in transcript testing. I'll create an issue for that. I'm not sure if it would be an easy or a hard fix.

[tleonhardt]

@kotfu

Issue #213 created to fix Cmd2TestCase so regexes with backslashes work.

If you have a good idea of how to fix it and are so inclined, that would rock. If not, I'll start looking into it.

Thanks for bringing this issue to our attention. I had noticed at one point that certain regexes didn't work properly with transcript testing, but my knowledge of regexes is weak enough that I wasn't 100% sure that I wasn't just doing something dumb.