Merge into master : integration-fixes : lp:~racb/git-ubuntu : Git : Code : git-ubuntu

Reviewer	Review Type	Date Requested	Status
Bryce Harrington			Approve on 2021-07-09
Server Team CI bot	continuous-integration		Approve on 2021-07-06
Christian Ehrhardt 		2021-06-21	Needs Fixing on 2021-06-30
Review via email: mp+404448@code.staging.launchpad.net

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2021-06-21:

#

PASSED: Continuous integration, rev:fa330f02f0fb5874d2af825d74e589e1988bbf3e
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/40/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: VM Reset
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/40//rebuild

review: Approve (continuous-integration)

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2021-06-29:

#

PASSED: Continuous integration, rev:42904d21aa2715441ececb96956491e46a7bb751
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/41/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: VM Reset
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/41//rebuild

review: Approve (continuous-integration)

Revision history for this message

Christian Ehrhardt  (paelzer) wrote on 2021-06-30:

#

Download full text (5.3 KiB)

7c43469 snapcraft: drop --enable-experimental-package-repositories

  Non important, but for proof this one could mention in the commit message where it was removed
   https://github.com/snapcore/snapcraft/commit/0662e630713b50ae1c4126eddb9b82fff133b6d0
   https://github.com/snapcore/snapcraft/pull/3520

2ac404f source_builder: fix test changelog sign-off line

That is indeed the right format, and the tests pass still

d8edae8 Add integration test for sed

  Indeed it matches 67206b06 ("Add awk integration test")
  But "awk ''" will return with a good RC, yet OTOH "sed ''" will hang waiting for input.
  In the test it behaves the same :-/

This hangs here at 2 and never reached 3
$ python3 -c 'import subprocess; print("1"); subprocess.check_call(["awk", ""]); print("2"); subprocess.check_call(["sed", ""]); print("3")'
1
2

I see the test bot passed and it does for
  $ python3.8 -m pytest gitubuntu/integration_test.py::test_sed_runnable).
This is due to the default capture settings in pytest, this one hangs:
  $ python3.8 -m pytest -s gitubuntu/integration_test.py::test_sed_runnable
So I wonder if we should here use the safer:
  subprocess.check_call(["sed", "--version"])

This works with both, otherwise you might one day need to debug anyother case, switch on -s to see the output and then this one blocks you hanging.

31dbb06 Fix and test update-maintainer integration

+1 to the dependency fix as it was formerly only a build-dependency

  dist-packages/debian/changelog.py is a bit unhappy with our test skeleton file:
    changelog.py:484: UserWarning: Unexpected line while looking for more change data or trailer:
    changelog.py:484: UserWarning: Found eof where expected more change data or trailer

  While those are only internal warnings and the test works fine they could later
  break on us without actually being a finding on git ubuntu.
  The following would fix this.

- -- Test Maintainer <email address hidden> Tue, 01 Jun 2021 14:09:49
+ -- Test Maintainer <email address hidden> Tue, 01 Jun 2021 14:09:49 +0000

fa330f0 Test and fix git-ubuntu.reconstruct-changelog

I saw these warnings:
   20: from gitubuntu.test_fixtures import pygit2_repo
     unused-import: Unused pygit2_repo imported from gitubuntu.test_fixtures
   167: def test_reconstruct_changelog(pygit2_repo, monkeypatch):
     redefinition of unused 'pygit2_repo' from line 20
But they seem to be a false positive of my checker.

Overall the integration of this test is more complex than the test itself and I appreaciate
the huge comment block above the tree/entry point checks.
The bot exercised case #5 and I did #4 and both work. I assume you have had a look
at the others.
This looks slightly over-complex still - why is ENTRY_POINT_MAP a cascaded map and
get_entry_point using the name argument?
Did this before need multiple dict entries?
You could (for a bit of simplification) drop that double-indirection.

But this commit LGTM +1 no changes needed unless you want to.

42904d2 Include all dpkg decompressors in the snap

I wondered about the .deb dependencies, but indirectly they will be in (by apt/dpkg)
...

7c43469 snapcraft: drop --enable-experimental-package-repositories

Non important, but for proof this one could mention in the commit message where it was removed
   https://github.com/snapcore/snapcraft/commit/0662e630713b50ae1c4126eddb9b82fff133b6d0
   https://github.com/snapcore/snapcraft/pull/3520

2ac404f source_builder: fix test changelog sign-off line

That is indeed the right format, and the tests pass still

d8edae8 Add integration test for sed

Indeed it matches 67206b06 ("Add awk integration test")
  But "awk ''" will return with a good RC, yet OTOH "sed ''" will hang waiting for input.
  In the test it behaves the same :-/

This hangs here at 2 and never reached 3
$ python3 -c 'import subprocess; print("1"); subprocess.check_call(["awk", ""]); print("2"); subprocess.check_call(["sed", ""]); print("3")'
1
2

I see the test bot passed and it does for
  $ python3.8 -m pytest gitubuntu/integration_test.py::test_sed_runnable).
This is due to the default capture settings in pytest, this one hangs:
  $ python3.8 -m pytest -s gitubuntu/integration_test.py::test_sed_runnable
So I wonder if we should here use the safer:
  subprocess.check_call(["sed", "--version"])

This works with both, otherwise you might one day need to debug anyother case, switch on -s to see the output and then this one blocks you hanging.

31dbb06 Fix and test update-maintainer integration

+1 to the dependency fix as it was formerly only a build-dependency

dist-packages/debian/changelog.py is a bit unhappy with our test skeleton file:
    changelog.py:484: UserWarning: Unexpected line while looking for more change data or trailer:  
    changelog.py:484: UserWarning: Found eof where expected more change data or trailer

While those are only internal warnings and the test works fine they could later
  break on us without actually being a finding on git ubuntu.
  The following would fix this.

- -- Test Maintainer <test@example.com>  Tue, 01 Jun 2021 14:09:49
+ -- Test Maintainer <test@example.com>  Tue, 01 Jun 2021 14:09:49 +0000

fa330f0 Test and fix git-ubuntu.reconstruct-changelog

I saw these warnings:
   20: from gitubuntu.test_fixtures import pygit2_repo                                  
     unused-import: Unused pygit2_repo imported from gitubuntu.test_fixtures
   167: def test_reconstruct_changelog(pygit2_repo, monkeypatch):                        
     redefinition of unused 'pygit2_repo' from line 20
 But they seem to be a false positive of my checker.

Overall the integration of this test is more complex than the test itself and I appreaciate
 the huge comment block above the tree/entry point checks.
 The bot exercised case #5 and I did #4 and both work. I assume you have had a look
 at the others.
 This looks slightly over-complex still - why is ENTRY_POINT_MAP a cascaded map and 
 get_entry_point using the name argument?
 Did this before need multiple dict entries?
 You could (for a bit of simplification) drop that double-indirection.

But this commit LGTM +1 no changes needed unless you want to.

42904d2 Include all dpkg decompressors in the snap

I wondered about the .deb dependencies, but indirectly they will be in (by apt/dpkg)
 and also via ubuntu-dev-tools which we now have.

I'm not entirely convinced on just calling them accepting bad RC and hoping that a
 real issue would raise a different exception.

How about calling this in pairs and verify input==output.
 This will catch unavailable binaries as well as changed behavior and unexpected error 
 conditions.

Here a suggestion for you that works for me:

--- a/gitubuntu/integration_test.py
+++ b/gitubuntu/integration_test.py
@@ -114,21 +114,25 @@ def test_sed_runnable():
     # The following list of decompressors is taken from the keys of $COMP in
     # scripts/dpkg/Compression.pm from the dpkg source. It may need to be
     # updated if future dpkg updates add additional compression mechanisms.
-    ('gunzip',),
-    ('bunzip2',),
-    ('unxz',),  # this serves both the xz and lzma compression types
+    (['gzip',  'gunzip'],),
+    (['bzip2', 'bunzip2'],),
+    (['xz',    'unxz'],),  # this serves both the xz and lzma compression types
 ])
 def test_dpkg_compressors(decompressor):
     '''The given dpkg decompressor should be available'''
-    # We can expect the decompressor process itself to return non-zero since
-    # they generally consider an empty file to be invalid input data. However
-    # the exec itself should succeed, and if fails we can expect to see an
-    # exception, such as a FileNotFoundError.
-    subprocess.call(
-        [decompressor],
-        stdin=subprocess.DEVNULL,
-        stdout=subprocess.DEVNULL,
-    )
+    # We pass data to compressor and related decompressor and verify
+    # if it still matches. This will catch unavailable binaries as well
+    # as changed behavior.
+    expected = b"Test input data\n"
+    test = subprocess.Popen("%s - | %s" % (decompressor[0], decompressor[1]),
+                            stdin=subprocess.PIPE,
+                            stdout=subprocess.PIPE,
+                            stderr=subprocess.PIPE,
+                            shell=True)
+    result = test.communicate(expected)
+    # Output is stdout/stderr tuple, we expect the data we have sent
+    # and nothing in stderr
+    assert (result == (expected, b''))
 
 
 def test_update_maintainer(tmpdir):

review: Needs Fixing

Revision history for this message

Bryce Harrington (bryce) wrote on 2021-06-30:

#

Christian already gave a very thorough review (appreciated!) I just wanted to add a few minor comments...

2ac404fe
- lgtm
- The deb-changelog man page doesn't mention the two-space requirement, though it does show it in its example. I couldn't find if this is specified somewhere or is just convention, but in either case this looks correct.

d8edae83
  - Just to confirm what Christian already said, the test will indeed hang:
    >>> import subprocess
    >>> subprocess.check_call(['sed', ''])

    hang hang
    hang hang
    0
  - I agree for simple sanity checking passing --version or --help may suffice.

31dbb06d
- The code docs for test_update_maintainer() have unbalanced double-quotes.
- Also, you've used single-quotes for code docs in the other test cases in this file, you might want to do the same with this one for consistency (or update the other test cases in this file to use double-quotes).

fa330f02
  - In the else branch of the if statement that checs os.environ to determine the ENTRY_POINT_DIR, after checking if bindir exists, you can just assign ENTRY_POINT_DIR = bindir.
  - Earlier in that if statement in the elif branch there's another os.path.join() that should probably also do a os.path.exists().
  - ENTRY_POINT_TYPE can be None, so when get_entry_point() uses it as a key to ENTRY_POINT_MAP, it could fail, so maybe use .get() or wrap the return in a try. I see you use .skipif() for the test case but it may be less complicated (and more future-proof) if None was handled in get_entry_point().
  - Since ENTRY_POINT_MAP seems to be used only in get_entry_point, could it just be moved internal to that function? Like Christian already pointed out, I'm also curious why this is a map of maps, when the sub-maps contain only a single entry and have the same key? I agree this would be worth flattening.

42904d21
- Back to single-quotes in the code docs. :-)
- I like Christian's suggested improvement to do paired compress/decompress, but I'd suggest this for the test parameterization:

+@pytest.mark.parametrize(['compressor', 'decompressor'], [
...
+ ('gzip', 'gunzip',),
+ ('bzip2', 'bunzip2',),
+ ('xz', 'unxz',), # this serves both the xz and lzma compression types
])

and then use variables compressor and decompressor instead of decompressor[0] and decompressor[0] respectively.

Christian already gave a very thorough review (appreciated!)  I just wanted to add a few minor comments...

2ac404fe
  - lgtm
  - The deb-changelog man page doesn't mention the two-space requirement, though it does show it in its example.  I couldn't find if this is specified somewhere or is just convention, but in either case this looks correct.

d8edae83
  - Just to confirm what Christian already said, the test will indeed hang:
    >>> import subprocess
    >>> subprocess.check_call(['sed', ''])

hang hang
    hang hang
    0
  - I agree for simple sanity checking passing --version or --help may suffice.

31dbb06d
  - The code docs for test_update_maintainer() have unbalanced double-quotes.
  - Also, you've used single-quotes for code docs in the other test cases in this file, you might want to do the same with this one for consistency (or update the other test cases in this file to use double-quotes).

fa330f02
  - In the else branch of the if statement that checs os.environ to determine the ENTRY_POINT_DIR, after checking if bindir exists, you can just assign ENTRY_POINT_DIR = bindir.
  - Earlier in that if statement in the elif branch there's another os.path.join() that should probably also do a os.path.exists().
  - ENTRY_POINT_TYPE can be None, so when get_entry_point() uses it as a key to ENTRY_POINT_MAP, it could fail, so maybe use .get() or wrap the return in a try.   I see you use .skipif() for the test case but it may be less complicated (and more future-proof) if None was handled in get_entry_point().
  - Since ENTRY_POINT_MAP seems to be used only in get_entry_point, could it just be moved internal to that function?  Like Christian already pointed out, I'm also curious why this is a map of maps, when the sub-maps contain only a single entry and have the same key?  I agree this would be worth flattening.

42904d21
  - Back to single-quotes in the code docs.  :-)
  - I like Christian's suggested improvement to do paired compress/decompress, but I'd suggest this for the test parameterization:

+@pytest.mark.parametrize(['compressor', 'decompressor'], [
...
+ ('gzip', 'gunzip',),
+ ('bzip2', 'bunzip2',),
+ ('xz', 'unxz',), # this serves both the xz and lzma compression types
])

and then use variables compressor and decompressor instead of decompressor[0] and decompressor[0] respectively.

review: Needs Fixing

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2021-07-06:

#

PASSED: Continuous integration, rev:24abd00a19ee5d14fb514d4fae03c2413ee6c143
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/42/
Executed test runs:
    SUCCESS: VM Setup
    SUCCESS: Build
    SUCCESS: VM Reset
    SUCCESS: Unit Tests
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/42//rebuild

review: Approve (continuous-integration)

Revision history for this message

Robie Basak (racb) wrote on 2021-07-06:

#

Download full text (10.8 KiB)

Thank you for the reviews!

I think I've addressed everything raised. In general I've adopted every
suggestion, except around get_entry_point and ENTRY_POINT_MAP which I've
explained below and in comments in the code instead.

Range diff: https://paste.ubuntu.com/p/tsTDnzWMkV/

> 7c43469 snapcraft: drop --enable-experimental-package-repositories
>
> Non important, but for proof this one could mention in the commit message where it was removed
> https://github.com/snapcore/snapcraft/commit/0662e630713b50ae1c4126eddb9b82fff133b6d0
> https://github.com/snapcore/snapcraft/pull/3520

OK, but this already landed in
https://code.launchpad.net/~paride/usd-importer/+git/usd-importer/+merge/403740.

> d8edae8 Add integration test for sed
>
> Indeed it matches 67206b06 ("Add awk integration test")
> But "awk ''" will return with a good RC, yet OTOH "sed ''" will hang waiting for input.
> In the test it behaves the same :-/
>
> This hangs here at 2 and never reached 3
> $ python3 -c 'import subprocess; print("1"); subprocess.check_call(["awk", ""]); print("2"); subprocess.check_call(["sed", ""]); print("3")'
> 1
> 2
>
> I see the test bot passed and it does for
> $ python3.8 -m pytest gitubuntu/integration_test.py::test_sed_runnable).
> This is due to the default capture settings in pytest, this one hangs:
> $ python3.8 -m pytest -s gitubuntu/integration_test.py::test_sed_runnable
> So I wonder if we should here use the safer:
> subprocess.check_call(["sed", "--version"])
>
> This works with both, otherwise you might one day need to debug anyother case, switch on -s to see the output and then this one blocks you hanging.

Good spot! Fixed.

> 31dbb06 Fix and test update-maintainer integration
>
> +1 to the dependency fix as it was formerly only a build-dependency
>
> dist-packages/debian/changelog.py is a bit unhappy with our test skeleton file:
> changelog.py:484: UserWarning: Unexpected line while looking for more change data or trailer:
> changelog.py:484: UserWarning: Found eof where expected more change data or trailer
>
> While those are only internal warnings and the test works fine they could later
> break on us without actually being a finding on git ubuntu.
> The following would fix this.
>
> - -- Test Maintainer <email address hidden> Tue, 01 Jun 2021 14:09:49
> + -- Test Maintainer <email address hidden> Tue, 01 Jun 2021 14:09:49 +0000

Fixed. Thanks!

> fa330f0 Test and fix git-ubuntu.reconstruct-changelog
>
> I saw these warnings:
> 20: from gitubuntu.test_fixtures import pygit2_repo
> unused-import: Unused pygit2_repo imported from gitubuntu.test_fixtures
> 167: def test_reconstruct_changelog(pygit2_repo, monkeypatch):
> redefinition of unused 'pygit2_repo' from line 20
> But they seem to be a false positive of my checker.

I think that's because of pytest's magic use of fixtures. They get used as test
function parameters, which isn't normal "use", so it appears to be unused.

> Overall the integration of this test is more complex than the test itself and I appreaciate
> the huge comment block above the tree/entry point checks.
> The bot exercised case #5 and I did #4 and b...

Thank you for the reviews!

I think I've addressed everything raised. In general I've adopted every
suggestion, except around get_entry_point and ENTRY_POINT_MAP which I've
explained below and in comments in the code instead.

Range diff: https://paste.ubuntu.com/p/tsTDnzWMkV/

> 7c43469 snapcraft: drop --enable-experimental-package-repositories
> 
>   Non important, but for proof this one could mention in the commit message where it was removed
>    https://github.com/snapcore/snapcraft/commit/0662e630713b50ae1c4126eddb9b82fff133b6d0
>    https://github.com/snapcore/snapcraft/pull/3520

OK, but this already landed in
https://code.launchpad.net/~paride/usd-importer/+git/usd-importer/+merge/403740.

> d8edae8 Add integration test for sed
> 
>   Indeed it matches 67206b06 ("Add awk integration test")
>   But "awk ''" will return with a good RC, yet OTOH "sed ''" will hang waiting for input.
>   In the test it behaves the same :-/
> 
>   This hangs here at 2 and never reached 3
> $ python3 -c 'import subprocess; print("1"); subprocess.check_call(["awk", ""]); print("2"); subprocess.check_call(["sed", ""]); print("3")'
> 1
> 2
> 
> I see the test bot passed and it does for
>   $ python3.8 -m pytest gitubuntu/integration_test.py::test_sed_runnable).
> This is due to the default capture settings in pytest, this one hangs:
>   $ python3.8 -m pytest -s gitubuntu/integration_test.py::test_sed_runnable
> So I wonder if we should here use the safer:
>   subprocess.check_call(["sed", "--version"])
> 
> This works with both, otherwise you might one day need to debug anyother case, switch on -s to see the output and then this one blocks you hanging.

Good spot! Fixed.

> 31dbb06 Fix and test update-maintainer integration
> 
>   +1 to the dependency fix as it was formerly only a build-dependency
> 
>   dist-packages/debian/changelog.py is a bit unhappy with our test skeleton file:
>     changelog.py:484: UserWarning: Unexpected line while looking for more change data or trailer:  
>     changelog.py:484: UserWarning: Found eof where expected more change data or trailer
> 
>   While those are only internal warnings and the test works fine they could later
>   break on us without actually being a finding on git ubuntu.
>   The following would fix this.
> 
> - -- Test Maintainer <test@example.com>  Tue, 01 Jun 2021 14:09:49
> + -- Test Maintainer <test@example.com>  Tue, 01 Jun 2021 14:09:49 +0000

Fixed. Thanks!

> fa330f0 Test and fix git-ubuntu.reconstruct-changelog
> 
>  I saw these warnings:
>    20: from gitubuntu.test_fixtures import pygit2_repo
>      unused-import: Unused pygit2_repo imported from gitubuntu.test_fixtures
>    167: def test_reconstruct_changelog(pygit2_repo, monkeypatch):
>      redefinition of unused 'pygit2_repo' from line 20
>  But they seem to be a false positive of my checker.

I think that's because of pytest's magic use of fixtures. They get used as test
function parameters, which isn't normal "use", so it appears to be unused.

>  Overall the integration of this test is more complex than the test itself and I appreaciate
>  the huge comment block above the tree/entry point checks.
>  The bot exercised case #5 and I did #4 and both work. I assume you have had a look
>  at the others.

Yes - I tested them manually. Actually when you build the snap using snap.sh
(as CI does it) the deb build (case 5) gets exercised as part of that. So it's
2 and 3 that need testing manually (and 4 gets tested naturally during
development iteration when I run tests by hand).

>  This looks slightly over-complex still - why is ENTRY_POINT_MAP a cascaded map and
>  get_entry_point using the name argument?
>  Did this before need multiple dict entries?
>  You could (for a bit of simplification) drop that double-indirection.

I added some comments to try to explain this better. When I say "entry points"
I mean others as well, but initially I'm only adding tests for one of them.
This is me adding tests for the bits that specifically failed and I fixed,
rather than trying to boil the ocean and adding tests for everything currently
untested all at once.

>  But this commit LGTM +1 no changes needed unless you want to.

> 42904d2 Include all dpkg decompressors in the snap
> 
>  I wondered about the .deb dependencies, but indirectly they will be in (by apt/dpkg)
>  and also via ubuntu-dev-tools which we now have.
> 
>  I'm not entirely convinced on just calling them accepting bad RC and hoping that a
>  real issue would raise a different exception.
> 
>  How about calling this in pairs and verify input==output.
>  This will catch unavailable binaries as well as changed behavior and unexpected error
>  conditions.
> 
> Here a suggestion for you that works for me:
> 
> --- a/gitubuntu/integration_test.py
> +++ b/gitubuntu/integration_test.py
> @@ -114,21 +114,25 @@ def test_sed_runnable():
>      # The following list of decompressors is taken from the keys of $COMP in
>      # scripts/dpkg/Compression.pm from the dpkg source. It may need to be
>      # updated if future dpkg updates add additional compression mechanisms.
> -    ('gunzip',),
> -    ('bunzip2',),
> -    ('unxz',),  # this serves both the xz and lzma compression types
> +    (['gzip',  'gunzip'],),
> +    (['bzip2', 'bunzip2'],),
> +    (['xz',    'unxz'],),  # this serves both the xz and lzma compression types
>  ])
>  def test_dpkg_compressors(decompressor):
>      '''The given dpkg decompressor should be available'''
> -    # We can expect the decompressor process itself to return non-zero since
> -    # they generally consider an empty file to be invalid input data. However
> -    # the exec itself should succeed, and if fails we can expect to see an
> -    # exception, such as a FileNotFoundError.
> -    subprocess.call(
> -        [decompressor],
> -        stdin=subprocess.DEVNULL,
> -        stdout=subprocess.DEVNULL,
> -    )
> +    # We pass data to compressor and related decompressor and verify
> +    # if it still matches. This will catch unavailable binaries as well
> +    # as changed behavior.
> +    expected = b"Test input data\n"
> +    test = subprocess.Popen("%s - | %s" % (decompressor[0], decompressor[1]),
> +                            stdin=subprocess.PIPE,
> +                            stdout=subprocess.PIPE,
> +                            stderr=subprocess.PIPE,
> +                            shell=True)
> +    result = test.communicate(expected)
> +    # Output is stdout/stderr tuple, we expect the data we have sent
> +    # and nothing in stderr
> +    assert (result == (expected, b''))
>  
>  
>  def test_update_maintainer(tmpdir):

Adopted, thanks. I also took Bryce's suggestion of explicit parameters for the
compressor and the decompressor, dropped some extra brackets, also asserted the
exit status being 0, and adjusted formatting/language slightly. I also used a
f-string instead of the %-expansion as that's cleaner and I'm using that more
in new code now. Finally, given we're testing actual compression and
decompression rather than just existence of the tool, I added a separate case
for the lzma mode inside xz, to parallel dpkg.

> Christian already gave a very thorough review (appreciated!) I just wanted to add a few minor comments...
> 
> 2ac404fe
>   - lgtm
>   - The deb-changelog man page doesn't mention the two-space requirement, though it does show it in its example. I couldn't find if this is specified somewhere or is just convention, but in either case this looks correct.
> 
> d8edae83
>   - Just to confirm what Christian already said, the test will indeed hang:
>     >>> import subprocess
>     >>> subprocess.check_call(['sed', ''])
> 
>     hang hang
>     hang hang
>     0
>   - I agree for simple sanity checking passing --version or --help may suffice.
> 
> 31dbb06d
>   - The code docs for test_update_maintainer() have unbalanced double-quotes.

Fixed.

>   - Also, you've used single-quotes for code docs in the other test cases in this file, you might want to do the same with this one for consistency (or update the other test cases in this file to use double-quotes).

Fixed.

> fa330f02
>   - In the else branch of the if statement that checs os.environ to determine the ENTRY_POINT_DIR, after checking if bindir exists, you can just assign ENTRY_POINT_DIR = bindir.

Good spot! This was indeed my intention when I factored out and created the
bindir variable, but I missed this instance. Fixed.

>   - Earlier in that if statement in the elif branch there's another os.path.join() that should probably also do a os.path.exists().

OK. I added an assert to ensure this, as it's not clear what to do if it
doesn't exist, but that should never happen. Then I thought that really this
assert should exist all the time, unless ENTRY_POINT_TYPE is None. So I added
that at the end.

>   - ENTRY_POINT_TYPE can be None, so when get_entry_point() uses it as a key to ENTRY_POINT_MAP, it could fail, so maybe use .get() or wrap the return in a try. I see you use .skipif() for the test case but it may be less complicated (and more future-proof) if None was handled in get_entry_point().

I take the point that get_entry_point() was error prone in the case that
ENTRY_POINT_TYPE is None, so I added an assertion and explained in the
docstring. I think I need to retain the skipIf though because otherwise running
the test in the case that entry points are not available makes no sense and I'd
prefer for the test to return "skipped" rather than "passed" in that case.

>   - Since ENTRY_POINT_MAP seems to be used only in get_entry_point, could it just be moved internal to that function?

It could be, but in my head the constants at the top apply to everything in the
file, which seems appropriate as ENTRY_POINT_MAP is within context for
potentially multiple tests.

>   Like Christian already pointed out, I'm also curious why this is a map of maps, when the sub-maps contain only a single entry and have the same key? I agree this would be worth flattening.

Explained in my response to Christian above. I also added comments as I'm
taking both of your queries as an indication that it wasn't explained well
enough in the code. I'm leaving the map of maps though, as this makes it easy
to incrementally add further integration tests for other entry points - at
which point that key would be used to differentiate. YAGNI would be a valid
argument against this, except that I've written it now, so factoring it out for
now only to refactoring it back later seems like a waste.

> 42904d21
>   - Back to single-quotes in the code docs. :-)
>   - I like Christian's suggested improvement to do paired compress/decompress, but I'd suggest this for the test parameterization:
> 
> +@pytest.mark.parametrize(['compressor', 'decompressor'], [
> ...
> + ('gzip', 'gunzip',),
> + ('bzip2', 'bunzip2',),
> + ('xz', 'unxz',), # this serves both the xz and lzma compression types
> ])
> 
> and then use variables compressor and decompressor instead of decompressor[0] and decompressor[0] respectively.

Agreed, and done.

Revision history for this message

Bryce Harrington (bryce) wrote on 2021-07-09:

#

Range diff LGTM, thanks for that.

As to ENTRY_POINT_MAP, I wondered if that might be foundation laying for future work, so ok by me to leave that as is for now. +1 to the added docs to explain the intent.

review: Approve

git-ubuntu

Merge ~racb/git-ubuntu:integration-fixes into git-ubuntu:master

Commit message

Description of the change

Preview Diff

Subscribers