ppa-dev-tools

Merge ppa-dev-tools:test-create-config into ppa-dev-tools:main

  1. Git
  2. lp:ppa-dev-tools
  3. test-create-config
  4. Merge into main
Proposed by Bryce Harrington
Status: Merged
Merge reported by: Bryce Harrington
Merged at revision: 6b06eaa91dc8931aea7767a9474a680412a0eff9
Proposed branch: ppa-dev-tools:test-create-config
Merge into: ppa-dev-tools:main
Diff against target: 275 lines (+97/-27)
4 files modified
ppa/ppa.py (+2/-0)
scripts/ppa (+21/-14)
tests/helpers.py (+16/-3)
tests/test_scripts_ppa.py (+58/-10)
Reviewer Review Type Date Requested Status
Athos Ribeiro (community) Approve
Alberto Contreras (community) Needs Information
Canonical Server Reporter Pending
Review via email: mp+438274@code.staging.launchpad.net

Description of the change

This adds unit tests for the create_config() command, which I thought might be a bit superfluous but it did uncover some bad handling of invalid inputs. There's a few other testsuite related fixes and cleanups included.

Best way to test this branch is to run the full testsuite:

    $ pytest-3

The current codebase had a test failure relating to processors; this fixes that, and all subsequent commits should likewise pass the full testsuite.

To post a comment you must log in.
Revision history for this message
Alberto Contreras (aciba) wrote :

Thanks, Bryce, for adding unit tests. The changes look good.

I have left some nits and two questions / concerns.

1) The main one is the inclusion of the mocking machinery in `Ppa.archive`.

This helps us to unit test the code and it is okay as is.

But, adds new code that it is not needed at runtime and could cause problems in the future. In this case, this is probably an exaggeration, as the modifications are simple and easy to track. But if we establish this pattern of allowing testing code within production code, things could get complicated and bite us at some point.

Have we thought about using other alternatives, as using `pytest.fixtures` and/or `unittests.mock` in the long term? Or are there restrictions to use those that I do not see?

2) The xfail comment.

review: Needs Information
Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

Thanks Bryce!

Other than Alberto's questions/comments (in special, the XFAIL one), this LGTM :)

Revision history for this message
Bryce Harrington (bryce) wrote :

Hi Alberto and Athos, thank you for reviewing! You're right the xfail can be dropped; I was using it to isolate some debugging work and appear to have forgotten to re-enable it.

Alberto, you're right that the _archive addition is a bit awkward. I took a fresh look this morning, and redid the code to avoid needing to add that, by creating the ArchiveMock just at the point of need, and avoid having to store it. For test code that should be fine, and it avoids impacting the run-time code, which could have side effects as you point out.

I also did the cleanups you suggested, squashed in the code review changes, and force pushed the branch. I think it looks a more solid, thank you both again for taking time to look at this.

Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

LGTM.

I just did not get why we needed the dict typecast there. Is it to satisfy type checking?

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

Thanks! Answer to the question below.

Revision history for this message
Bryce Harrington (bryce) wrote :

Landed:

To git+ssh://git.launchpad.net/ppa-dev-tools
   555b089..7e59879 main -> main

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

to all changes: