search

archlinux-downgrade / downgrade

Downgrade packages in Arch Linux
GNU General Public License v2.0
570 stars 24 forks source link

Integrate `pacignore` into `downgrade` #197

Closed atreyasha closed 2 years ago

atreyasha commented 2 years ago

Checklist

Description

To work towards smaller and more compact PRs, I moved (and split) a commit from the as/pacignore branch to this branch. This PR proposes the following:

  1. Use pacignore directly in downgrade as a form of integration
  2. Simplify unit tests of downgrade to no longer test features of what is now pacignore. Instead, we only test integration between the two scripts.

Pending

atreyasha commented 2 years ago

Wanted to get your advice on something.

ATM, I am masking the pacignore call from downgrade during testing to keep the latter's unit tests simple:

https://github.com/archlinux-downgrade/downgrade/blob/76b894cf9481376e867f21f99f75475aa681e86b/test/helper.sh#L31

Ideally, I would like to not do this and instead test the full pacignore call from downgrade. However, this would require a more complicated diff to the downgrade tests such as updating the path to include pacignore and overriding the write_pacman_conf function for this specific test case with something like this below:

https://github.com/archlinux-downgrade/downgrade/blob/76b894cf9481376e867f21f99f75475aa681e86b/test/helper_pacignore.sh#L8-L12

I see two possible options going forward:

  1. Implement this diff now to have a proper integration test between downgrade and pacignore
  2. Wait until we split downgrade into lower-level scripts and then implement integration tests for all lower-level scripts called from downgrade

An advantage of (1) is that we have the integration test done to check for bugs during development. A disadvantage of (1) is that it might involve work that we would discard later with the downgrade split, since we would anyways be moving most tests from function-level to script-level (as per my understanding).

WDYT?

pbrisbin commented 2 years ago

This is a tricky problem with no real objective answer. There's a lot written about the "testing pyramid" and the difference/usefulness/cost-of-maintenace-of unit-level tests vs integration-level tests.

What you describe, where you test downgrade and let it call a real pacignore, would be an integration test. It gives you high assurance but also higher cost. A bug in pacignore will materialize as a test failure (or failures) in dowgrade tests likely focused on other areas. They're typically slower to run and (related to the first point) failures can be hard to understand and take longer to fix -- they rarely point you at the bug, they just tell you there is a bug somewhere. Because of these downsides, it's not always strictly better to do this kind of testing.

Unit tests on the other hand are as the test is now: you are testing behavior in the downgrade "unit" and stubbing out pacignore. Presumably, pacignore itself is well-tested directly, so you can assume it behaves correctly and your stub is not hiding a bug. A step up from that would be to mock pacignore, and assert in your downgrade test that it's actually invoked with the arguments you expect.

Unit tests with mocking would describe something like:

Notice there is not a test like "Downgrade doesn't call pacignore add if the package is already in pacman.conf". This is because that crosses the boundary. pacignore check should have its own test that confirms that. Downgrade's responsibility is to change its behavior in response to pacignore check; it doesn't and shouldn't know why check returns true/false.

These unit tests with mocking would be my ideal, but it's not easy in a shell project. Particularly, even if you can somehow wrestle cram and bash into testing this way, tests that aren't maintainable because they're so obtuse overshadow the usefulness of following this philosophy -- it's just a bad outcome with more steps. So, personally, I think the current approach is actually the best trade-off between test coverage and maintenance headache, but I'm happy to defer to you and/or continue to help you explore these alternatives for yourself.

atreyasha commented 2 years ago

Thanks for explaining this in detail.

These unit tests with mocking would be my ideal, but it's not easy in a shell project.

Yes, that sounds like the best case scenario but I also see what you mean about the additional complexity involved.

but I'm happy to defer to you and/or continue to help you explore these alternatives for yourself

I'll think a bit more about this and see if there could be an easy + maintainable way of having unit tests for downgrade + mocking for lower-level scripts. Otherwise, we can still work with what we have now.

Thanks again, wishing you a nice Friday! :)