[Buildroot] [PATCH v5 04/10] testing/tests/download: add git hash test

[Ricardo Martincoski]

Hello,

On Mon, Feb 04, 2019 at 01:52 PM, Arnout Vandecappelle wrote:

> On 12/05/2018 04:58, Ricardo Martincoski wrote:
> [snip]
>>  support/testing/tests/download/gitremote.py   |  44 ++++++++++++++++++
>>  support/testing/tests/download/test_git.py    |  43 +++++++++++++++++
> 
>  flake8 says:
> 
> support/testing/tests/download/gitremote.py:5:1: I100 Import statements are in
> the wrong order. 'import infra' should be before 'import pexpect' and in a
> different group.
> support/testing/tests/download/test_git.py:4:1: I100 Import statements are in
> the wrong order. 'from gitremote import GitRemote' should be before 'import
> infra' and in a different group.
> support/testing/tests/download/test_git.py:4:1: I201 Missing newline between
> import groups. 'from gitremote import GitRemote' is identified as Third Party
> and 'import infra' is identified as Third Party.
> 
>  I fixed this while applying.

So you have the plugin flake8-import-order.
I don't have it locally and we don't have it yet in the docker image.
But thank you for fixing.

[snip]
>> +        daemon_cmd = ["git", "daemon", "--reuseaddr", "--verbose", "--listen=localhost", "--export-all",
> 
>  Line is too long. Fixed as well.

But it is shorter than 132 (as we set in .flake8), no?

Any chance you are using some version of flake8/pycodestyle that does not
support configs in .flake8 or max-line-length=132 ?

> 
>> +                      "--base-path={}".format(serveddir)]
>> +        for port in range(GIT_REMOTE_PORT_INITIAL, GIT_REMOTE_PORT_LAST + 1):
>> +            cmd = daemon_cmd + ["--port={port}".format(port=port)]
>> +            self.logfile.write("> starting git remote with '{}'\n".format(" ".join(cmd)))
>> +            self.daemon = pexpect.spawn(cmd[0], cmd[1:], logfile=self.logfile)
>> +            ret = self.daemon.expect(["Ready to rumble",
>> +                                      "Address already in use"])
> 
>  Shouldn't we add a timeout here, just to be safe?

I think the expect() method uses the default from the class if not defined.
And the default timeout for spawn is 30 seconds AFAICR.
I did not double-checked the docs yet.

[snip]
>> +class GitTestBase(infra.basetest.BRTest):
>> +    config = \
>> +        """
>> +        BR2_BACKUP_SITE=""
>> +        """
>> +    gitremotedir = infra.filepath("tests/download/git-remote")
>> +    gitremote = None
>> +
>> +    def setUp(self):
>> +        super(GitTestBase, self).setUp()
>> +        self.gitremote = GitRemote(self.builddir, self.gitremotedir, self.logtofile)
>> +
>> +    def tearDown(self):
>> +        self.show_msg("Cleaning up")
>> +        if self.gitremote:
>> +            self.gitremote.stop()
>> +        if self.b and not self.keepbuilds:
> 
>  This is actually a bit useless, since self.b is not initialzed in
> BRTest.__init__ you'll actually get either an exception or self.b evaluates to
> True. But the same pattern is used in other places, so OK. Can be fixed in a
> follow-up patch.

Probably some leftover from a rebase. I will look into it.


Regards,
Ricardo