Merge ~racb/git-ubuntu:test-optimization into git-ubuntu:master

Proposed by Robie Basak
Status: Needs review
Proposed branch: ~racb/git-ubuntu:test-optimization
Merge into: git-ubuntu:master
Diff against target: 340 lines (+105/-57)
4 files modified
gitubuntu/git_repository_test.py (+6/-6)
gitubuntu/repo_builder.py (+22/-11)
gitubuntu/repo_builder_test.py (+14/-14)
gitubuntu/source_builder.py (+63/-26)
Reviewer Review Type Date Requested Status
Bryce Harrington Needs Fixing
Server Team CI bot continuous-integration Approve
Review via email: mp+401673@code.staging.launchpad.net

Commit message

Make Jenkins happy

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:2ecc23b7c32eb2b78287700fa6538c217f249411
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/37/
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/37//rebuild

review: Approve (continuous-integration)
Revision history for this message
Bryce Harrington (bryce) wrote :

a375ea332b0d366eed7c284f6a093dd59e6d0917
  + LGTM

05a41143196849de5cc22b049b84cd5883469724
  + LGTM

d7d06fec7f4b96ec1173446579c0582a4d60bb87
  + LGTM

0790066a0b3bb109d95c1fe26d092475a9c433bc
  + sp. excecutable
  + It's good you're calling out the necessity of debian/rules being executable, however the explanation is leaving me with more questions. E.g. why must it be executable (nothing in the code seems to actually use it)? Why does it say "Exceptionally", by this do you just mean "Notably", or is an exception involved, or...? Why would the API need to represent debian/rule's executable nature? Should the code include an assertion check for execute permissions on the file? Yet I see the caller is explicitly setting the executable bit *after* the call... I'm guessing what you mean to say is that the routine does not preserve debian/rules' executable nature so callers will need to set that manually... or something like that? Perhaps you could clarify the text to explain it more precisely.
  + What should the behavior be if source/format, control, rules, or changelog were missing from the debian directory? I'm guessing it'll throw an exception on None.encode()
  + Technically the rtype is an generator, although I don't know if we make the documentation reflect that, but it can matter depending on how the routine's called

2ecc23b7c32eb2b78287700fa6538c217f249411
  + 98s -> 32s nice!
  + "But I'll leave this for another time." -> sounds like a separate bug report should be filed?
  + to_git_tree() needs its repo param documented
  + self.source.to_git_tree(repo) can fail from an assert in the internal write(). You might want to consider revising the logic so it falls back to the original method if the git tree write fails for whatever reason. I.e.:

        if not self.patches_applied:
            # Fast path if we don't need to apply patches (which is not yet
            # supported by source_builder.Source.to_git_tree()).
            try:
                return self.source.to_git_tree(repo)
            except:
                # On any error, fallback to original method
                pass

        with self.source as dsc_path:
            oid_str = gitubuntu.importer.dsc_to_tree_hash(
                repo,
                dsc_path,
                True,
            )
        return pygit2.Oid(binascii.unhexlify(oid_str))

('unhexlify' is a fun word to say.)

I notice there are no new or expanded test cases accompanying this. Do you feel the existing test cases still adequately exercise the code? to_git_tree() and (maybe) _to_path_bytes_pairs() are new, and there's a new code path in _obj_to_oid().

review: Needs Fixing

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches