git-ubuntu

Code review comment for ~racb/git-ubuntu:integration-fixes

Revision history for this message

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

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)
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):

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

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

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

« Back to merge proposal