Skip to content
This repository has been archived by the owner on Apr 4, 2019. It is now read-only.

Decode git outputs in order to fix bb stage #147

Merged
merged 4 commits into from
Jan 19, 2018
Merged

Conversation

miiila
Copy link
Contributor

@miiila miiila commented Jan 10, 2018

bb stage is not working with python3, This PR tries to fix it.

@miiila miiila requested review from Almad and abtris January 10, 2018 15:53
abtris
abtris previously approved these changes Jan 10, 2018
kylef
kylef previously approved these changes Jan 10, 2018
@kylef
Copy link
Member

kylef commented Jan 10, 2018

Good catch, @miiila might be a good opportunity to add test coverage for these units to ensure they work on all environments.

If interested you can adapt this test I wrote before:

class GitReleaserTestCase(unittest.TestCase):
    # Initialisation

    def test_errors_when_non_master(self):
        with temp_directory():
            subprocess.check_output('git init', shell=True)

            touch('README.md')
            subprocess.check_output('git add README.md', shell=True)
            subprocess.check_output('git commit -m initial', shell=True)

            subprocess.check_output('git checkout -b kylef', shell=True)

            with self.assertRaises(Exception):
                GitReleaser()

Some of the used utilities by this test:

import os
import tempfile
import shutil
import subprocess


class temp_directory(object):
    def __enter__(self):
        self.working_directory = os.getcwd()
        self.pathname = tempfile.mkdtemp()
        os.chdir(self.pathname)
        return self.pathname

    def __exit__(self, typ, value, traceback):
        os.chdir(self.working_directory)
        shutil.rmtree(self.pathname)


def touch(filename, contents=''):
    path, _ = os.path.split(filename)
    if len(path) > 0:
        os.makedirs(path)

    with open(filename, 'w') as fp:
        fp.write(contents)

@miiila miiila dismissed stale reviews from kylef and abtris via 3a6aee4 January 11, 2018 14:55
test/test_git.py Outdated

@patch('subprocess.check_output')
def test_branch_name_parsing(self, check_output_mock):
branch_name = 'prefix/branch-name'
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified by using b'prefix/branch-name' since this would work on both Pythons, on Python 3 it would be bytes and 2 a string.

check_output_mock.return_value = b'prefix/branch-name\n'
assert_equal('prefix/branch-name', get_current_branch())

it could fail for non-ASCII characters and check_call looks like capable of handling unicode input
@miiila
Copy link
Contributor Author

miiila commented Jan 11, 2018

@kylef thanks for the suggestion. I added (and simplified) tests and removed explicit str conversion. Could you please have a look? Thx.

@@ -1,5 +1,5 @@
import re
from subprocess import check_output
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't know why people hate this :)

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 found I have to change it because of the testing. When I patched the check_output and kept import, it was calling an original function rather then a mock.

Copy link
Contributor

Choose a reason for hiding this comment

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

...right, true

Copy link
Contributor

Choose a reason for hiding this comment

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

Never met anyone hating this :)

@@ -5,7 +5,7 @@


def deploy_staging():
branch_name = str(get_current_branch())
branch_name = get_current_branch()

post_message("Deploying branch %s to staging" % branch_name, "#deploy-queue")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you either need to encode here, or in post_message.

Copy link
Contributor

Choose a reason for hiding this comment

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

...oh, it's going just to slack, so it's probably better to encode at the output layer.

👍

@miiila miiila merged commit 217fbcd into master Jan 19, 2018
@miiila miiila deleted the miiila/fix-bb-stage branch January 19, 2018 13:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants