Skip to content

Atexit fix#328

Merged
tartley merged 3 commits intotartley:masterfrom
3tilley:atexit-fix
Jun 14, 2022
Merged

Atexit fix#328
tartley merged 3 commits intotartley:masterfrom
3tilley:atexit-fix

Conversation

@3tilley
Copy link
Copy Markdown
Contributor

@3tilley 3tilley commented Oct 28, 2021

A library we use throws this annoying error when it's called in under certain conditions. It's probably doing something wrong, but I think colorama should be more resilient to this particular bad state. I've attached the script that demonstrates the error. I've tried to write a test for this, but the best I can to do honestly test the functionality is to spin up a new Python in a subprocess that calls colorama, this seems awkward and was difficult to make portable. If it's your desire I'll do it though.

from __future__ import print_function

import codecs
import sys

from colorama import init

init()
sys.stdout = codecs.getwriter("utf-8")(sys.stdout.detach())

print("foo", sys.stdout)

Before and after the fix:
image

image

Rejected test attempt:

    def testDetachAfterInitDoesntThrow(self):
        fd, name = tempfile.mkstemp(suffix=".py", text=True)
        sample_python = """
from __future__ import print_function

import codecs
import sys

from colorama import init

init()
sys.stdout = codecs.getwriter("utf-8")(sys.stdout.detach())

print("Hello", sys.stdout)
        """
        temp_script = Path(name)
        with temp_script.open(mode="w") as f:
            f.write(sample_python)
        output = subprocess.check_output(["python", str(temp_script)], universal_newlines=True)
        print(output)
        assert "buffer" not in output

@3tilley
Copy link
Copy Markdown
Contributor Author

3tilley commented Oct 29, 2021

@tartley do you might approving me?

@tartley
Copy link
Copy Markdown
Owner

tartley commented Oct 29, 2021

Hi there! Thanks for the contribution, this is going to be a great fix.

I can't approve until we have a working test of some sort though. My motivation is that future contributors need some automated way for them to be notified if they make a change which completely breaks your change, so that we don't then deploy their broken change because we don't know about it.

How about something like the following? The first test demonstrates the existing AttributeError handling, and the second one demonstrates the ValueError that you are adding:

class StreamWrapperTest(TestCase):

    ...

    def test_closed_shouldnt_raise_on_closed_stream(self):
        stream = StringIO()
        stream.close()
        wrapper = StreamWrapper(stream, None)
        self.assertEqual(wrapper.closed, True)

    def test_closed_shouldnt_raise_on_detached_stream(self):
        stream = TextIOWrapper(StringIO())
        stream.detach()
        wrapper = StreamWrapper(stream, None)
        self.assertEqual(wrapper.closed, True)

As always, you can tell whether these tests are actually working using TDD, or simulating a bit artifically in this case, since we already have the code changed:

  1. remove the try...finally from StreamWrapper.closed
  2. add the tests and run them, they should both fail
  3. reintroduce the try...finally, only handling AttributeError
  4. rerun the tests, the first should pass
  5. make your change, adding the ValueError
  6. rerun the tests, they should both pass

@tartley tartley added the 0.4.5 Get merged for 0.4.5 release label Oct 29, 2021
@3tilley
Copy link
Copy Markdown
Contributor Author

3tilley commented Oct 29, 2021 •
edited
Loading

Great idea, added. I tried something similar before but wasn't able to detach a StringIO - I clearly don't have your deep stream knowledge!

@3tilley
Copy link
Copy Markdown
Contributor Author

3tilley commented Nov 3, 2021

I think this is just waiting a CI run, @tartley do you mind kicking it off?

@tartley tartley merged commit edf05b7 into tartley:master Jun 14, 2022
@3tilley
Copy link
Copy Markdown
Contributor Author

3tilley commented Jun 14, 2022

Thanks so much! I literally looked at this in an output today and thought back to this PR

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

Reviewers

No reviews

Assignees

No one assigned

Labels

0.4.5 Get merged for 0.4.5 release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@3tilley @tartley @Timokasse