GNOME Bugzilla – Bug 708069
admin: add U-Boot backend test case
Last modified: 2013-09-21 19:50:48 UTC
Since OSTree has a test suite, it is a good practice to add a test case for every new added feature. That was not the case when U-Boot backend support was added in commit 750a60d3a ("main: Add U-Boot bootlader backend support"). This patch-set does a little refactoring to libtest and add a test for this case. It is composed of the following patches: [PATCH 1/3]: tests: don't create a syslinux stub unconditionally in libtest [PATCH 2/3]: tests: allow setup_os_repository to create U-Boot stub config [PATCH 3/3]: admin: add U-Boot backend test case
Created attachment 254908 [details] [review] [PATCH 1/3] tests: don't create a syslinux stub unconditionally in libtest
Created attachment 254909 [details] [review] [PATCH 2/3] tests: allow setup_os_repository to create U-Boot stub config
Created attachment 254910 [details] [review] [PATCH 3/3] admin: add U-Boot backend test case
Review of attachment 254908 [details] [review]: Looks good.
Review of attachment 254909 [details] [review]: Also looks fine.
Review of attachment 254910 [details] [review]: Might as well squash this with the previous commit. There's a fair amount of duplication with the other admin test; but you've also changed things enough that it probably doesn't make sense to try to create helper functions yet. When OSTree supports even more complex deployment configuration (for example, a config file saying how many to retain and such), then we can look at a more abstract test suite.
Hi Colin, thanks a lot for your feedback. (In reply to comment #6) > Review of attachment 254910 [details] [review]: > > Might as well squash this with the previous commit. > Sorry if I break my changes in too many patches. I'm just used to other FOSS projects (e.g: Linux kernel) where that is considered a good practice since it makes easier to review and do incremental merges. There isn't such a thing as too many patches in the kernel as long as you don't break git bisectability but I'll squash the last two patches and submit a new patch-set if you prefer that way. > There's a fair amount of duplication with the other admin test; but you've also > changed things enough that it probably doesn't make sense to try to create > helper functions yet. When OSTree supports even more complex deployment > configuration (for example, a config file saying how many to retain and such), > then we can look at a more abstract test suite. Yeah, initially I wanted to do some refactoring to avoid code duplication but then I noticed that there is also a fair amount of duplication between test-admin-deploy-1.sh and test-admin-deploy-2.sh too (which btw could be renamed to something more descriptive). So I preferred to keep this change as less intrusive as possible and thought that wouldn't be a blocker for this test to be merged. But maybe is a good opportunity to create some helper functions in libtest.sh that can be used for all test-admin-deploy* instead introducing yet more code duplication.
(In reply to comment #7) > Hi Colin, thanks a lot for your feedback. > > (In reply to comment #6) > > Review of attachment 254910 [details] [review] [details]: > > > > Might as well squash this with the previous commit. > > > > Sorry if I break my changes in too many patches. I'm just used to other FOSS > projects (e.g: Linux kernel) where that is considered a good practice since it > makes easier to review and do incremental merges. Definitely, I like splitting too; I guess a rule of thumb is that if you're adding an API for which there will likely only be one consumer in the forseeable future, then it's usually fine to squash the addition of that API with its consumer. > Yeah, initially I wanted to do some refactoring to avoid code duplication but > then I noticed that there is also a fair amount of duplication between > test-admin-deploy-1.sh and test-admin-deploy-2.sh too (which btw could be > renamed to something more descriptive). Right...not sure what to call it since -1 just got so unwieldy. > So I preferred to keep this change as less intrusive as possible and thought > that wouldn't be a blocker for this test to be merged. But maybe is a good > opportunity to create some helper functions in libtest.sh that can be used for > all test-admin-deploy* instead introducing yet more code duplication. Ok yep! We can come back to it later. Thanks for adding tests, very much appreciated!
The patches have been already merged and the test case was added in commit 3715b0cc8 ("admin: add U-Boot backend test case") so this bug can be closed.