| Message ID | 20180512025833.22998-5-ricardo.martincoski@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | tests for git download infra v5 | expand |
Hi Ricardo, I'll apply this today, but I have some comment... 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. [snip] > diff --git a/support/testing/tests/download/gitremote.py b/support/testing/tests/download/gitremote.py > new file mode 100644 > index 0000000000..60bc49fbf8 > --- /dev/null > +++ b/support/testing/tests/download/gitremote.py > @@ -0,0 +1,44 @@ > +# subprocess does not kill the child daemon when a test case fails by raising > +# an exception. So use pexpect instead. > +import pexpect > + > +import infra > + > +GIT_REMOTE_PORT_INITIAL = 9418 > +GIT_REMOTE_PORT_LAST = GIT_REMOTE_PORT_INITIAL + 99 > + > + > +class GitRemote(object): > + def __init__(self, builddir, serveddir, logtofile): > + """ > + Start a local git server. > + > + In order to support test cases in parallel, select the port the > + server will listen to in runtime. Since there is no reliable way > + to allocate the port prior to starting the server (another > + process in the host machine can use the port between it is > + selected from a list and it is really allocated to the server) > + try to start the server in a port and in the case it is already > + in use, try the next one in the allowed range. > + """ > + self.daemon = None > + self.port = None > + self.logfile = infra.open_log_file(builddir, "gitremote", logtofile) > + > + daemon_cmd = ["git", "daemon", "--reuseaddr", "--verbose", "--listen=localhost", "--export-all", Line is too long. Fixed as well. > + "--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? This can be fixed in a follow-up patch. > + if ret == 0: > + self.port = port > + return > + raise SystemError("Could not find a free port to run git remote") > + > + def stop(self): > + if self.daemon is None: > + return > + self.daemon.terminate(force=True) > diff --git a/support/testing/tests/download/test_git.py b/support/testing/tests/download/test_git.py > new file mode 100644 > index 0000000000..14fc8e4da3 > --- /dev/null > +++ b/support/testing/tests/download/test_git.py > @@ -0,0 +1,43 @@ > +import os > + > +import infra > +from gitremote import GitRemote > + > + > +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. Regards, Arnout > + self.b.delete() > + > + def check_hash(self, package): > + # store downloaded tarball inside the output dir so the test infra > + # cleans it up at the end > + env = {"BR2_DL_DIR": os.path.join(self.builddir, "dl"), > + "GITREMOTE_PORT_NUMBER": str(self.gitremote.port)} > + self.b.build(["{}-dirclean".format(package), > + "{}-source".format(package)], > + env) > + > + > +class TestGitHash(GitTestBase): > + br2_external = [infra.filepath("tests/download/br2-external/git-hash")] > + > + def test_run(self): > + with self.assertRaises(SystemError): > + self.check_hash("bad") > + self.check_hash("good") > + self.check_hash("nohash") >
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
On 05/02/2019 01:52, Ricardo Martincoski wrote: > 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. We could add it, but in fact a lot of things "break" with it. > 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 ? No, I just manually check in my editor. The idea is that our target line length is 80, but in many cases lines are difficult to split (it looks worse). Since we have the flake8 run in our CI, we shouldn't error out on too long lines. And adding an exception comment for each long line does not make the code more readable. So as a compromise, we set the line length to 132 in .flake8. But the intention is to have line length of 80. Perhaps we should remove it from flake8, and instead add the option to .gitlab-ci.yml.in. Hm, yes, that would make a lot of sense... > >> >>> + "--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. I had only diagonally read the documentation and I saw that timeout in expect() defaults to -1, so I assumed it was infinity... But in fact it indeed means "inherit from the spawn class", and there it defaults to 30. Regards, Arnout > > [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.
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: Hi, >>> 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. > I had only diagonally read the documentation and I saw that timeout in expect() > defaults to -1, so I assumed it was infinity... But in fact it indeed means > "inherit from the spawn class", and there it defaults to 30. I got confused by that as well when I wrote the docker/compose tests. Notice that we set timeout to 5 in emulator.py: timeout=5 * self.timeout_multiplier So I believe -1 means 5 seconds, not 30.
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e80491cdde..27b0255343 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -263,6 +263,7 @@ tests.core.test_rootfs_overlay.TestRootfsOverlay: *runtime_test tests.core.test_timezone.TestGlibcAllTimezone: *runtime_test tests.core.test_timezone.TestGlibcNonDefaultLimitedTimezone: *runtime_test tests.core.test_timezone.TestNoTimezone: *runtime_test +tests.download.test_git.TestGitHash: *runtime_test tests.fs.test_ext.TestExt2: *runtime_test tests.fs.test_ext.TestExt2r1: *runtime_test tests.fs.test_ext.TestExt3: *runtime_test diff --git a/support/testing/tests/download/__init__.py b/support/testing/tests/download/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/support/testing/tests/download/br2-external/git-hash/Config.in b/support/testing/tests/download/br2-external/git-hash/Config.in new file mode 100644 index 0000000000..e69de29bb2 diff --git a/support/testing/tests/download/br2-external/git-hash/external.desc b/support/testing/tests/download/br2-external/git-hash/external.desc new file mode 100644 index 0000000000..41316c8b25 --- /dev/null +++ b/support/testing/tests/download/br2-external/git-hash/external.desc @@ -0,0 +1 @@ +name: GIT_HASH diff --git a/support/testing/tests/download/br2-external/git-hash/external.mk b/support/testing/tests/download/br2-external/git-hash/external.mk new file mode 100644 index 0000000000..4646dfe2b0 --- /dev/null +++ b/support/testing/tests/download/br2-external/git-hash/external.mk @@ -0,0 +1,4 @@ +include $(sort $(wildcard $(BR2_EXTERNAL_GIT_HASH_PATH)/package/*/*.mk)) + +# Get the git server port number from the test infra +GITREMOTE_PORT_NUMBER ?= 9418 diff --git a/support/testing/tests/download/br2-external/git-hash/package/bad/bad.hash b/support/testing/tests/download/br2-external/git-hash/package/bad/bad.hash new file mode 100644 index 0000000000..b9e1baec84 --- /dev/null +++ b/support/testing/tests/download/br2-external/git-hash/package/bad/bad.hash @@ -0,0 +1 @@ +sha256 0000000000000000000000000000000000000000000000000000000000000000 bad-a238b1dfcd825d47d834af3c5223417c8411d90d.tar.gz diff --git a/support/testing/tests/download/br2-external/git-hash/package/bad/bad.mk b/support/testing/tests/download/br2-external/git-hash/package/bad/bad.mk new file mode 100644 index 0000000000..5497bd6bfe --- /dev/null +++ b/support/testing/tests/download/br2-external/git-hash/package/bad/bad.mk @@ -0,0 +1,10 @@ +################################################################################ +# +# bad +# +################################################################################ + +BAD_VERSION = a238b1dfcd825d47d834af3c5223417c8411d90d +BAD_SITE = git://localhost:$(GITREMOTE_PORT_NUMBER)/repo.git + +$(eval $(generic-package)) diff --git a/support/testing/tests/download/br2-external/git-hash/package/good/good.hash b/support/testing/tests/download/br2-external/git-hash/package/good/good.hash new file mode 100644 index 0000000000..9e92ab8ab9 --- /dev/null +++ b/support/testing/tests/download/br2-external/git-hash/package/good/good.hash @@ -0,0 +1 @@ +sha256 d00ae598e9e770d607621a86766030b42eaa58156cb8d482b043969da7963c23 good-a238b1dfcd825d47d834af3c5223417c8411d90d.tar.gz diff --git a/support/testing/tests/download/br2-external/git-hash/package/good/good.mk b/support/testing/tests/download/br2-external/git-hash/package/good/good.mk new file mode 100644 index 0000000000..0f0eefd944 --- /dev/null +++ b/support/testing/tests/download/br2-external/git-hash/package/good/good.mk @@ -0,0 +1,10 @@ +################################################################################ +# +# good +# +################################################################################ + +GOOD_VERSION = a238b1dfcd825d47d834af3c5223417c8411d90d +GOOD_SITE = git://localhost:$(GITREMOTE_PORT_NUMBER)/repo.git + +$(eval $(generic-package)) diff --git a/support/testing/tests/download/br2-external/git-hash/package/nohash/nohash.mk b/support/testing/tests/download/br2-external/git-hash/package/nohash/nohash.mk new file mode 100644 index 0000000000..1da19d88c6 --- /dev/null +++ b/support/testing/tests/download/br2-external/git-hash/package/nohash/nohash.mk @@ -0,0 +1,10 @@ +################################################################################ +# +# nohash +# +################################################################################ + +NOHASH_VERSION = a238b1dfcd825d47d834af3c5223417c8411d90d +NOHASH_SITE = git://localhost:$(GITREMOTE_PORT_NUMBER)/repo.git + +$(eval $(generic-package)) diff --git a/support/testing/tests/download/git-remote/repo.git/.gitattributes b/support/testing/tests/download/git-remote/repo.git/.gitattributes new file mode 100644 index 0000000000..eb50c64a21 --- /dev/null +++ b/support/testing/tests/download/git-remote/repo.git/.gitattributes @@ -0,0 +1 @@ +objects/*/* binary diff --git a/support/testing/tests/download/git-remote/repo.git/HEAD b/support/testing/tests/download/git-remote/repo.git/HEAD new file mode 100644 index 0000000000..cb089cd89a --- /dev/null +++ b/support/testing/tests/download/git-remote/repo.git/HEAD @@ -0,0 +1 @@ +ref: refs/heads/master diff --git a/support/testing/tests/download/git-remote/repo.git/config b/support/testing/tests/download/git-remote/repo.git/config new file mode 100644 index 0000000000..07d359d07c --- /dev/null +++ b/support/testing/tests/download/git-remote/repo.git/config @@ -0,0 +1,4 @@ +[core] + repositoryformatversion = 0 + filemode = true + bare = true diff --git a/support/testing/tests/download/git-remote/repo.git/objects/99/f2e3e1cb15f9b52fa29f66d380dda061d917ab b/support/testing/tests/download/git-remote/repo.git/objects/99/f2e3e1cb15f9b52fa29f66d380dda061d917ab new file mode 100644 index 0000000000000000000000000000000000000000..9db72668cf9374b0b85a25a19f30084fd460072d GIT binary patch literal 49 zcmb<m)YkO!4K*-JGB7bPFg6VIIDN)5wZcw)UHz=nDZYB`lCt5lYr9_mN?Omvz#_so F4FHVv5b^*3 literal 0 HcmV?d00001 diff --git a/support/testing/tests/download/git-remote/repo.git/objects/a2/38b1dfcd825d47d834af3c5223417c8411d90d b/support/testing/tests/download/git-remote/repo.git/objects/a2/38b1dfcd825d47d834af3c5223417c8411d90d new file mode 100644 index 0000000000000000000000000000000000000000..31b6bcf34de247d14531c70b36e28d895ba20af3 GIT binary patch literal 152 zcmV;J0B8Sr0j<tW4#FT1Kw;OMVlL1IsFY%i@d9r21jApQv_OKm@b)%w;oi9Y7BA10 zl&FE!)2`JJz?dk*5QMWrMPrqWC`}wkKTO<v<fHILXtWHU?OrNe$zk;cE?667R~`$& zv3{^mUp&tVY3*G}ClHEjqx7`b(D8wDw&^y}!k==CGBv`Lu^Y`f_^!m*+1KHhS@s5O G+F(P1IZe3$ literal 0 HcmV?d00001 diff --git a/support/testing/tests/download/git-remote/repo.git/objects/e7/9c5e8f964493290a409888d5413a737e8e5dd5 b/support/testing/tests/download/git-remote/repo.git/objects/e7/9c5e8f964493290a409888d5413a737e8e5dd5 new file mode 100644 index 0000000000000000000000000000000000000000..df2037a2d2d30afb20e31cd558c92c2edabe7cf6 GIT binary patch literal 23 fcmb<m^geacKWIb3i8H5lPWqgA$i$$b%kl;Qg;xrl literal 0 HcmV?d00001 diff --git a/support/testing/tests/download/git-remote/repo.git/refs/heads/master b/support/testing/tests/download/git-remote/repo.git/refs/heads/master new file mode 100644 index 0000000000..b6bccc1c17 --- /dev/null +++ b/support/testing/tests/download/git-remote/repo.git/refs/heads/master @@ -0,0 +1 @@ +a238b1dfcd825d47d834af3c5223417c8411d90d diff --git a/support/testing/tests/download/gitremote.py b/support/testing/tests/download/gitremote.py new file mode 100644 index 0000000000..60bc49fbf8 --- /dev/null +++ b/support/testing/tests/download/gitremote.py @@ -0,0 +1,44 @@ +# subprocess does not kill the child daemon when a test case fails by raising +# an exception. So use pexpect instead. +import pexpect + +import infra + +GIT_REMOTE_PORT_INITIAL = 9418 +GIT_REMOTE_PORT_LAST = GIT_REMOTE_PORT_INITIAL + 99 + + +class GitRemote(object): + def __init__(self, builddir, serveddir, logtofile): + """ + Start a local git server. + + In order to support test cases in parallel, select the port the + server will listen to in runtime. Since there is no reliable way + to allocate the port prior to starting the server (another + process in the host machine can use the port between it is + selected from a list and it is really allocated to the server) + try to start the server in a port and in the case it is already + in use, try the next one in the allowed range. + """ + self.daemon = None + self.port = None + self.logfile = infra.open_log_file(builddir, "gitremote", logtofile) + + daemon_cmd = ["git", "daemon", "--reuseaddr", "--verbose", "--listen=localhost", "--export-all", + "--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"]) + if ret == 0: + self.port = port + return + raise SystemError("Could not find a free port to run git remote") + + def stop(self): + if self.daemon is None: + return + self.daemon.terminate(force=True) diff --git a/support/testing/tests/download/test_git.py b/support/testing/tests/download/test_git.py new file mode 100644 index 0000000000..14fc8e4da3 --- /dev/null +++ b/support/testing/tests/download/test_git.py @@ -0,0 +1,43 @@ +import os + +import infra +from gitremote import GitRemote + + +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: + self.b.delete() + + def check_hash(self, package): + # store downloaded tarball inside the output dir so the test infra + # cleans it up at the end + env = {"BR2_DL_DIR": os.path.join(self.builddir, "dl"), + "GITREMOTE_PORT_NUMBER": str(self.gitremote.port)} + self.b.build(["{}-dirclean".format(package), + "{}-source".format(package)], + env) + + +class TestGitHash(GitTestBase): + br2_external = [infra.filepath("tests/download/br2-external/git-hash")] + + def test_run(self): + with self.assertRaises(SystemError): + self.check_hash("bad") + self.check_hash("good") + self.check_hash("nohash")