launchpad-reviewers team mailing list archive

[Yuliy Schwartzburg <mp+481785@xxxxxxxxxxxxxxxxxx>]

Some questions inline

Diff comments:

> diff --git a/lib/lp/bugs/utilities/tests/test_filebugdataparser.py b/lib/lp/bugs/utilities/tests/test_filebugdataparser.py
> index 28d4be9..84b8e0b 100644
> --- a/lib/lp/bugs/utilities/tests/test_filebugdataparser.py
> +++ b/lib/lp/bugs/utilities/tests/test_filebugdataparser.py
> @@ -44,9 +67,65 @@ class TestFileBugDataParser(TestCase):
>          self.assertEqual(b"", parser._consumeBytes(b"0"))
>          self.assertEqual(b"", parser._consumeBytes(b"0"))
>  
> +    def test__findLineBreakType(self):
> +        # _findLineBreakType reads the whole message until either an LF or
> +        # a CRLF is found, returns that type in bytes string format and exits.
> +
> +        msg = dedent(
> +            """\
> +            Header: value
> +            Space-Folded-Header: this header
> +             is folded with a space.
> +            Tab-Folded-Header: this header
> +            \tis folded with a tab.
> +            Another-header: another-value
> +
> +            Not-A-Header: not-a-value
> +            """
> +        )
> +
> +        # Test the above message with LF endings
> +        msg_with_lf = msg.encode("ASCII")
> +        lf_parser = FileBugDataParser(MockBytesIO(msg_with_lf))
> +        lf_linebreak = lf_parser._findLineBreakType()
> +
> +        self.assertEqual(lf_linebreak, b"\n")

Can we add a warning if you do use \n and test that the warning is sent? Or will that be part of another MP?

> +
> +        # Test the above message after replacing LF with CRLF
> +        msg_with_crlf = msg.replace("\n", "\r\n").encode("ASCII")
> +        crlf_parser = FileBugDataParser(MockBytesIO(msg_with_crlf))
> +        crlf_linebreak = crlf_parser._findLineBreakType()
> +
> +        self.assertEqual(crlf_linebreak, b"\r\n")
> +
> +        # Test the above message after deleting the line-breaks
> +        # Which should break as there must be at least some line-breaks
> +        # in the message

Are we sure there are always line breaks in the message? I guess yes because of headers right?

> +        msg_without_linebreak = msg.replace("\n", "").encode("ASCII")
> +        without_linebreak_parser = FileBugDataParser(
> +            MockBytesIO(msg_without_linebreak)
> +        )
> +
> +        self.assertRaisesWithContent(
> +            AssertionError,
> +            "There are no linebreaks in the blob.",
> +            without_linebreak_parser._findLineBreakType,
> +        )
> +
> +        # Test the above message after replacing LF with CR
> +        # Which should break as we do not accept CR line-breaks
> +        msg_with_cr = msg.replace("\n", "\r").encode("ASCII")
> +        cr_parser = FileBugDataParser(MockBytesIO(msg_with_cr))
> +
> +        self.assertRaisesWithContent(
> +            AssertionError,
> +            "The wrong linebreak is used. CR isn't accepted.",
> +            cr_parser._findLineBreakType,
> +        )
> +
>      def test_readLine(self):
>          # readLine reads a single line of the file.
> -        parser = FileBugDataParser(io.BytesIO(b"123\n456\n789"))
> +        parser = FileBugDataParser(MockBytesIO(b"123\n456\n789"))
>          self.assertEqual(b"123\n", parser.readLine())
>          self.assertEqual(b"456\n", parser.readLine())
>          self.assertEqual(b"789", parser.readLine())


-- 
https://code.launchpad.net/~ilkeremrekoc/launchpad/+git/launchpad/+merge/481785
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilkeremrekoc/launchpad:accept-crlf-bugreports into launchpad:master.