Add test for \r in directory parsing

arti!1048 needed to be sure to know how carriage returns are handled in C Tor, so this test attempts to do the same. This MR is created in order to add another good test case to C Tor as a byproduct.

requested review from @nickm

mentioned in merge request arti!1048

I think it might be worth writing the behaviour in the spec.

Personally I don't think ignoring \r is the best behaviour if we were starting from scratch but if this is how it works now, we probably don't want to change it.

Also, now that we know that \r is just filtered out, and is not an EOL for the directory info, is it still worth it to writing this behaviour in the spec?

To make sure that \r is really getting treated as an EOL here (and not just getting treated as whitespace at the end of the string) I think it makes sense to have two tokens in the string, so that we can make sure that they don't get treated as one.

In other words, we should also test how these strings behave:

• "uptime 1024\ruptime 2048"
• "uptime 1024\r\nuptime 2048"

From these 2 tests, I found that \r\n is indeed treated as a EOL, but not \r alone. It seems \r is indeed NOT a EOL character.

I will just add the \r\n test case if it still seems worthwhile to add. This also means arti!1048 needs more work

changed this line in version 2 of the diff

So, what I was hoping for was to have a test for all of the behavior here: not just for \r, not just for \r\n, but for both. But if I understand your changed code, you no longer have a test for \r alone -- only a test for \r\n.

Ah, I was having some problems representing \r in the test: if I had a line with \r, and tried to say there were 2 tokens in that line, C Tor would crash while running the tests, but if I said there was one line it would still work. I was unsure if the latter would be acceptable, but perhaps that doesn't matter when trying to define expected behaviour in this case. I'll add that test back in, expecting only one token to be parsed.

added 1 commit

• c5889d3f - Test \r more effectively using 2 lines

Compare with previous version

added 1 commit

• 738785ea - Test both \r and \r\n with the expected failure conditions added

Compare with previous version

This looks good now; thanks! I'll approve, and @dgoulet can merge when he's back from vacation.

(We should also probably decide whether this behavior is as-intended, or whether it's an accident of using isspace() here.)

approved this merge request

(We should also probably decide whether this behavior is as-intended, or whether it's an accident of using isspace() here.)

Can we make a decision on this? arti!1048 is blocked on it.

(My personal inclination is that \r ought to be rejected unless we think there are any things which generate docs with \r in. Including HS descriptors...)

merged