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

Fix #119 #120

Merged
merged 3 commits into from
Jan 10, 2020
Merged

Fix #119 #120

merged 3 commits into from
Jan 10, 2020

Conversation

lubomir
Copy link
Contributor

@lubomir lubomir commented Oct 1, 2019

When there is an incomplete multibyte sequence, process the data only until the start of this sequence. When next chunk is read, prepend the left overs to it. This should complete the sequence and processing should continue normally.

Fixes: #119

@coveralls
Copy link

coveralls commented Oct 1, 2019

Pull Request Test Coverage Report for Build 175

  • 16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 53.512%

Totals Coverage Status
Change from base Build 174: 0.1%
Covered Lines: 3200
Relevant Lines: 5980

💛 - Coveralls

@lubomir
Copy link
Contributor Author

lubomir commented Oct 1, 2019

Well, the test is failing for me locally with tox. Not sure why Travis is working.

@alexandrevicenzi
Copy link
Contributor

@lubomir the test should fail on Travis?

Couple things to notice, you probably are on Fedora, Travis probably on Ubuntu, also, locale and/or language settings may differ from your distro to Travis.

@@ -199,6 +199,12 @@ def test_run_show_cmd_logfile_stdout(self, mock_out):
self.assertEqual(mock_out.getvalue(),
'COMMAND: echo foo\n-----------------\nfoo\n')

def test_run_split_in_middle_of_utf8_sequence(self):
logfile = os.path.join(self.tmp_dir, 'output.log')
cmd = "printf ' ' && printf 'č%.0s' {1..10000}"
Copy link
Member

Choose a reason for hiding this comment

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

Apparently this isn't working properly in Travis because that {1..10000} syntax is a feature of bash, not supported by the dash shell used by Debian & Ubuntu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that makes sense. I'll rework to avoid that then.

@@ -199,6 +199,12 @@ def test_run_show_cmd_logfile_stdout(self, mock_out):
self.assertEqual(mock_out.getvalue(),
'COMMAND: echo foo\n-----------------\nfoo\n')

def test_run_split_in_middle_of_utf8_sequence(self):
Copy link
Member

Choose a reason for hiding this comment

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

If you're only planning to add the test now and not a fix, would you like to add an xfail marker here? Otherwise we can't merge it without breaking CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please don't merge this. I wanted provide a reproducer. I may get to writing a fix for this eventually, but not sure when.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the xfail. If you want to, feel free to merge.

@@ -199,6 +201,13 @@ def test_run_show_cmd_logfile_stdout(self, mock_out):
self.assertEqual(mock_out.getvalue(),
'COMMAND: echo foo\n-----------------\nfoo\n')

@pytest.mark.xfail(reason="Not fixed yet (#119)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Add strict=True to xfail

@lubomir lubomir changed the title Add failing test for unicode character on chunk boundary Fix #119 Oct 2, 2019
@lubomir
Copy link
Contributor Author

lubomir commented Oct 2, 2019

I added another commit that seems to fix the issue (at least it makes the test pass). I'm not sure if it's the best possible fix though.

When there is an incomplete multibyte sequence, process the data only
until the start of this sequence. When next chunk is read, prepend the
left overs to it. This should complete the sequence and processing
should continue normally.

Fixes: release-engineering#119
@rohanpm rohanpm merged commit 047e9ed into release-engineering:master Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in shortcuts.run when unicode character falls on chunk boundary
4 participants