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 tests for issue #355 #426

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

add tests for issue #355 #426

wants to merge 6 commits into from

Conversation

niccokunzmann
Copy link
Member

@niccokunzmann niccokunzmann commented Oct 3, 2022

see #356
see #355

@NicoHood - I created this PR to make sure the tests work.

@niccokunzmann
Copy link
Member Author

@NicoHood I cherry-picked your commit from #356 and there is a test failing for a content line.

    def test_escaping(self):
        # verify that escaped non safe chars are decoded correctly
        NON_SAFE_CHARS = ',\\;:'
        for char in NON_SAFE_CHARS:
            cn_escaped = "Society\\%s 2014" % char
            cn_decoded = "Society%s 2014" % char
            vevent = Event.from_ical(
                'BEGIN:VEVENT\r\n'
                'ORGANIZER;CN=%s:that\r\n'
                'END:VEVENT\r\n' % cn_escaped
            )
>           self.assertEqual(vevent['ORGANIZER'].params['CN'], cn_decoded)
E           AssertionError: ['Society\\', ' 2014'] != 'Society, 2014'

It looks like a string gets split into parts if it has a space....

@@ -0,0 +1,8 @@
BEGIN:VEVENT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this file added? It seems completely identical to your test file

@NicoHood
Copy link
Contributor

NicoHood commented Oct 3, 2022

The string does not get split into parts if it has a space. You can clearly see the space in your result. It is split, when a backslash is being escaped.

I currently try to understand why, but the test itself looks like a horrible syntax. Can we please fix it and make it use python fstrings?

@NicoHood
Copy link
Contributor

NicoHood commented Oct 3, 2022

Alright, so I can see what is happening here.

This is the failing test:

And this is the function that splits up our string:

def from_ical(cls, st, strict=False):

It will recognize CN=Society\, 2014 as the input string and split it accordingly by the comma. That is how multiple parameters should be split.

The RFC section that describes this process is this one:
https://icalendar.org/iCalendar-RFC-5545/3-1-content-lines.html

It seems that I have not implemented it correctly and for me it is still a brainfuck to understand how it should be implemented. Which strings can be quoted and which not. I think this needs more detailed parsing, but we can get to that.

@NicoHood
Copy link
Contributor

NicoHood commented Oct 3, 2022

So I looked at the mentioned spec and I could not find any part, where escaping of those characters in the test are described. I am wondering where this was defined, in which spec? Otherwise I am unable to fix anything here.

This is what I understood from the spec:

  • The name can only contain alphanumeric characters
  • Multiple parameters can follow
  • Those parameters can be quoted with double quotes (case sensitive) or without quotes (case insensitive)
  • No matter what the parameters cannot single escape comma, backslash, etc. by using backslashes. Only with double quotes they can be used.
  • After the parameters follows the value

It seems in my implementation I made the mistake to quote those characters by using a backslash.

But the test also looks incorrect. If those parameter value are not quoted (which they are not) they should be split by a comma. And that is what the code does now.

I have to think about this some more, but there seems to be a lot wrong. Before my PR but also with. We need to document the code much more, it is totally confusing and not understandable.

@NicoHood
Copy link
Contributor

NicoHood commented Oct 3, 2022

Maybe we should just take the abnf and convert it to a regex? It might be a bad idea, I dont know yet, but the abnf itself should be as close to the spec as we can get. https://github.com/aas-core-works/abnf-to-regexp

@niccokunzmann
Copy link
Member Author

niccokunzmann commented Oct 4, 2022

Hm... Which interface do we have to fulfill for parsing these parameters?
Would you like to help create a list of examples? Then, we can feed them into a test function... If the test is wrong in testing what it does, we can change it but I would like to be more precise than the test before.

This is what I understood from the spec:

Can you link that here?

Here is a list of tests that can be written and parametrized with different values to make sure it is working.

  • The name can only contain alphanumeric characters
    • test valid names
    • test invalid characters (hypothesis)
  • Multiple parameters can follow
    • test 0
    • test 1
    • test 2
    • test with broken one in the middle
  • Those parameters can be quoted with double quotes (case sensitive)
    • test case sensitivity
    • test conversion to ical again to have quotes
  • without quotes (case insensitive)
    • test case insensitivity
    • test conversion to ical again to have no quotes
  • (L1) No matter what the parameters cannot single escape comma, backslash, etc. by using backslashes.
  • (L2) Only with double quotes they can be used.
  • After the parameters follows the value
    • empty value, other values

(1) One proposal would be that I write tests and you can use TDD to work a bit more on the code.

(2) Another proposal is to just fix this little test to be accurate: Is the problem that the current implementation does not distinct between (L1) and (L2) above?

@NicoHood
Copy link
Contributor

NicoHood commented Oct 7, 2022

Yeah, if you can write tests first, that would help me a lot. This way we can talk clearer about the spec and how it should look like.

@niccokunzmann
Copy link
Member Author

niccokunzmann commented Oct 7, 2022 via email

@NicoHood
Copy link
Contributor

NicoHood commented Oct 8, 2022

I will have a look at it this week. I will let you know of my progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants