revise storage: better test loading, shared disk selection
ClosedPublic

Authored by adamwill on Jul 24 2015, 11:14 PM.

Details

Summary

This contains several tweaks to storage handling. It adds a
method for disk selection which all the storage tests can
share. It sets up a more extensible approach for main.pm to
run the storage tests, instead of an ever-growing forest of
'else' clauses. Finally it sets up a couple of methods for
changing partitioning schemes on the custom part screen and
uses one of them in the software RAID test; the other will
be used for other custom storage tests.

This kills the two_disks needle. I could keep it and work
it into select_disks, but it doesn't fit naturally and I
really just don't see the point of the needle. The only thing
we lose is we don't check that anaconda actually sees two
disks in the 'attach two disks, only install to one' test
(that's server_sata_multi), but the other multi-disk tests
will serve to catch that case failing for some reason.

What I actually intended to do was add some more tests for
different custom part storage types, but it seemed a good
idea to do some of this cleanup so that can be implemented
efficiently. I'll have followups for that.

Test Plan

Run all tests and ensure they work exactly as
before (not just that they still pass, but that the correct
test steps are actually scheduled in each case.)

Diff Detail

Repository
rOPENQATESTS os-autoinst-distri-fedora
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
adamwill retitled this revision from to revise storage: better test loading, shared disk selection.Jul 24 2015, 11:14 PM
adamwill updated this object.
adamwill edited the test plan for this revision. (Show Details)
adamwill added reviewers: jskladan, garretraziel.

Note that select_disks() intentionally does not click Done; this is so tests that need to do other stuff on the INSTALLATION DESTINATION screen before clicking Done can.

My first design for this was a bit different. It would have ditched all the different storage tests - disk_guided_* and disk_custom_* - and instead had tests tied to the actual screens (INSTALLATION DESTINATION, Installation Options, reclaim space, and the custom part screen) which would have different behaviour depending on the variables set for the test. But as I started writing that it started to feel like it'd get too messy - I quite like the idea of the tests defining various variables to specify what behaviour they want and the tests actually implementing it, but even just the INSTALLATION DESTINATION test would've been a sea of 'if this var then click that', and the more tests we implement the worse it'd get.

So I went with this somewhat less ambitious hybrid approach where there are still lots of tests, but they can use shared methods to share common work, and there's the somewhat-clever bits in main.pm to avoid a sea of elsif there. As we add more storage tests we can see if it holds up, I guess.

jskladan requested changes to this revision.Jul 27 2015, 11:52 AM
jskladan added inline comments.
lib/anacondalog.pm
60–82

Although I get the overall need for the method, it should be written a bit differently.
At the moment, the select_disks(), select_disks(0), and select_disks(1) have the same outcome - the first disk will be selected. The difference between ...(0) and ...(1) is in the "external context" - that is, when anaconda sees one disk, it is automagically selected in the spoke, with multiple disks, you need to click on the appropriate one to choose it.
This should be of no importance to the developer, otherwise the method has no other use than possible code de-duplication (and even that could be argued about).
Without the knowledge of Anaconda behaviour, one could even reasonable expect, that select_disks(0) will end up with no disks selected.

Please change the method to get rid of the ...(0) x ...(1) problem. Either make the ...(0) invalid, or (rather) make it so it ends up with no selected disks.

On top of that, I think it would be quite useful to implement the method rather like`select_disks([n, n, n])`, that would be able to select n-th disk in particular - there are needles for selecting each n-th disk anyway, and since we are implementing utility method...

70–72

Since the "Done" button needs to be clicked in the test anyway, please move this back to disk_guided_empty.pm. I know that this has the theoretical advantage of being able to check for the pony icon with any disk setup, but it IMHO is not something we need to be able to do anyway, and it unnecessarily clutters the method.

83–96

Please provide at least some needles to go with the code, or remove the methods.

main.pm
93

Could this be written any less in perl, please? At least add a comment.

On top of that - I'd rather the test execution to fail, than fall back to guided_empty if the appropriate DISK_GUIDED file is not found. That could easily lead to many false positives.

125–137

As above - this will most probably lead to false positives - let the process fail, instead of fallbacking to "safe" default.

tests/disk_guided_delete_partial.pm
8–9

Please add comments describing that you now "select the first disk" as it was in the original file.

tests/disk_guided_empty.pm
16

Add comment about what's happening here.

tests/disk_guided_encrypted.pm
8–9

ditto

tests/disk_guided_multi.pm
8–9

ditto

tests/disk_guided_multi_empty_all.pm
8–9

ditto

This revision now requires changes to proceed.Jul 27 2015, 11:52 AM

Thanks for the comments!

So, on the select_disks thing, there's kind of an ambiguity. If you consider it from the point of view of 'how many disks are selected after I call this function?' you're sort of right - except only sort of, because if you call select_disks() or select_disks(0) with more than one disk attached to the system, you wind up with 0 disks selected, not 1.

But I guess I was conceiving of it in a different way: to me it was, I suppose, explicitly_select_disks() - i.e. the parameter doesn't express how many disks will be selected after the method is run, which is unavoidably subject to other factors, but how many disks the method itself will actively try to select.

The problem with rewriting it as you suggest is it makes the method more complex. If we make it so the parameter always indicates how many disks will be selected after it's run, not how many the method will explicitly select, we need to teach the method about anaconda's handling of single vs. multiple disk situations, whereas right now it's the tests that know about that (and call the method appropriately) - which to my mind is cleaner, because the tests already have the single vs. multi-disk case difference kind of 'baked in', they don't need any conditional logic.

I'll look at alternatives today, but I reserve the right to maintain that the current implementation is the best, perhaps with a rename of the method somehow.

adamwill added inline comments.Jul 27 2015, 3:38 PM
lib/anacondalog.pm
83–96

custom_change_type is already used for software RAID. I was already working on adding tests for btrfs, thinp and the alternative filesystems - which is why I started working on this stuff in the first place - I just ran out of time. I think it probably makes more sense for the needles for new device type tests to go in the commits for those tests, rather than in this commit; this is setting up the framework, the other commits will add the implementation.

main.pm
93

This is already the improved version, you should've seen the first one...I'm not sure what's so difficult about it? Just the -e? It's the same as bash, it just means "if it exists". I can add a comment, sure. I can make it if (-e $loc) { $storage = $loc; } spread across three lines, but god, I hate that style for simple little things.

The problem with the failing is that we have the test disk_guided_free_space, which has a post-install test but for the actual installation portion expects to just use disk_guided_empty. Note I'm just following the existing behaviour, here - look left and note the comment # also DISK_GUIDED_FREE_SPACE.

125–137

The reason for this is simple: several storage tests have no postinstall test because they need none.

To implement things as you want, because we have both:

  • tests that need a 'during install' test but no 'post install' test
  • tests that need a 'post install' test but no 'during install' test

we'd have to have separate DISK_CUSTOM_POST and DISK_GUIDED_POST variables, or I suppose make the value for the DISK_CUSTOM and DISK_GUIDED variables into some more complex form (a list?) I'm not sure either of those is any better than the behaviour as I wrote it.

In D475#8927, @adamwill wrote:

The problem with rewriting it as you suggest is it makes the method more complex. If we make it so the parameter always indicates how many disks will be selected after it's run, not how many the method will explicitly select, we need to teach the method about anaconda's handling of single vs. multiple disk situations, whereas right now it's the tests that know about that (and call the method appropriately) - which to my mind is cleaner, because the tests already have the single vs. multi-disk case difference kind of 'baked in', they don't need any conditional logic.

In my opinion, if we want to extract common functionality into generic magic functions, we should let functions to do the *complete* magic - to be actually useful like "if I will call this function, I know for sure that X will be true" than to be just sort of macros like "this thing repeats a lot so I will introduce new method so I will type less".

Note that we have already sort of done it with "login into terminal as root" functionality - we have root_console method that universally gives us TTY with root logged in (and it does a lot of complicated things). It's true that it introduces complexity into newly created methods - but for the whole sake of reducing complexity in tests (and I would think that that's the whole point).

Think about someone who will want to help us with writing tests (for example pschindl). It will be much easier to explain/document that calling select_disks(X) will result in X disks selected (and it will be much more clearer in test code also) than to explain/document that the result of select_disks() is the same as select_disks(0) and that's one disk selected when only one disk is attached and none disk selected when multiple disks are attached, while calling select_disks(1) will fail when only one disk attached and will result on one disk selected when multiple disks are attached. I would think that it's better when you can easily explain outcome of called method than if you have to explain what method does in it's body.

The last thing is that current behaviour requires user to constantly think about context - because number of disks used is set in the different place (environment variable in WebUI).

garretraziel added inline comments.Jul 28 2015, 8:57 AM
lib/anacondalog.pm
83–96

Then I would add this method with first commit that requires it; but I am okay with leaving it here.

main.pm
93

You could use:

$storage = $loc if (-e $loc);

(I actually kinda hate that Perl doesn't have "explicit is better than implicit" principle)

As with the failing, problem is that when someone makes a typo - he for example sets "DISK_GUIDED=free_spece", it doesn't fail for him (all tests may actually pass!), so if he will not be careful (and don't notice that different test was loaded in WebUI), he will be oblivious that he made a typo. I'm actually more inclined to duplicate whole disk_guided_empty into disk_guided_free_space than let tests silently pass when someone makes a typo.

And at the same time, I'm not saying that existing behaviour doesn't have this flaw also :-).

125–137

You are right, there are instances when we don't need any postinstall tests and not failing when there is no postinstall test makes sense here.

Moreover, whether user hadn't made a typo would have been already checked by correctly loading preinstall test.

In D475#8927, @adamwill wrote:

The problem with rewriting it as you suggest is it makes the method more complex. If we make it so the parameter always indicates how many disks will be selected after it's run, not how many the method will explicitly select, we need to teach the method about anaconda's handling of single vs. multiple disk situations, whereas right now it's the tests that know about that (and call the method appropriately) - which to my mind is cleaner, because the tests already have the single vs. multi-disk case difference kind of 'baked in', they don't need any conditional logic.

In my opinion, if we want to extract common functionality into generic magic functions, we should let functions to do the *complete* magic - to be actually useful like "if I will call this function, I know for sure that X will be true" than to be just sort of macros like "this thing repeats a lot so I will introduce new method so I will type less".

Note that we have already sort of done it with "login into terminal as root" functionality - we have root_console method that universally gives us TTY with root logged in (and it does a lot of complicated things). It's true that it introduces complexity into newly created methods - but for the whole sake of reducing complexity in tests (and I would think that that's the whole point).

Think about someone who will want to help us with writing tests (for example pschindl). It will be much easier to explain/document that calling select_disks(X) will result in X disks selected (and it will be much more clearer in test code also) than to explain/document that the result of select_disks() is the same as select_disks(0) and that's one disk selected when only one disk is attached and none disk selected when multiple disks are attached, while calling select_disks(1) will fail when only one disk attached and will result on one disk selected when multiple disks are attached. I would think that it's better when you can easily explain outcome of called method than if you have to explain what method does in it's body.

The last thing is that current behaviour requires user to constantly think about context - because number of disks used is set in the different place (environment variable in WebUI).

I absolutely understand what you're saying and it's a valid point of view, I'm still not sure I agree, though :)

I guess I worry about abstracting too far away from what we're actually *doing*. We're still fundamentally writing tests based on performing a specific series of steps in a UI, and the UI step this method is concerned with is 'go to the INSTALLATION DESTINATION screen and make some choices'. I'm just not sure it's actually wise to have the function abstract away the fact that the INSTALLATION DESTINATION screen works in a specific way, when part of what we're *doing* is testing the operation of that screen.

I should really emphasize that the method name select_disks was fairly haphazardly chosen, my thought process was not "write a method to select some disks". I actually started out to write a method whose job would be to handle the entire INSTALLATION DESTINATION screen (up to clicking 'Done' on it), but eventually downgraded it a bit to just doing the most common choices and not clicking 'Done' because I didn't want a whole series of possibilities for clicking the encryption button, clicking the bootloader button, clicking the 'I want to reclaim space' button, etc etc. Still, the idea behind the method is 'provide a common method for going to ID and doing some stuff', it's not really designed to be 'select X disks'.

Having said that, I'm planning to spend some time fiddling about with it and see if I can't come up with something we'd all be happy with; I was going to do it yesterday but got diverted by other stuff.

adamwill added inline comments.Jul 28 2015, 3:48 PM
main.pm
93

(on the $storage thing) - sure, that works, I always forget about that style for some reason. Mine is basically bash style, I guess.

So I have two concerns with the default value thing:

  1. If we're going to change it, I'd prefer to do that in a separate commit, to make it clear that this commit aims to reimplement the existing logic in a more efficient way, whereas that one changes the logic because we decided it was bad.
  1. What if we wind up having other GUIDED tests of the same kind - they just want to click through the screen but do something different because of how the test box is configured? Just copying the test seems like an okay tradeoff for *one* case, but what about two, three, four, five...do we remember that the tests are just copies of each other or do they start bitrotting? Still, I don't really mind the idea. Obviously I'd still want to use disk_guided_empty if DISK_GUIDED isn't set at all.
In D475#8927, @adamwill wrote:

... doesn't express how many disks will be selected after the method is run, which is unavoidably subject to other factors...

I don't see a reason to having it in a function, then, at all.

The problem with rewriting it as you suggest is it makes the method more complex.

I don't see that as a problem - abstracting stuff to functions is (to some extent) about code deduplication, but also (mostly) about the ability to make things smarted.

If we make it so the parameter always indicates how many disks will be selected after it's run, not how many the method will explicitly select, we need to teach the method about anaconda's handling of single vs. multiple disk situations.

Once again - for me, the worse problem is that in order to use the method, you need to know about anaconda's implementation. _That_ is what feels wrong. On top of that, having the method "smart" (in the 'select x disks') is better in the long term - if anaconda changes the way it currently behaves (e.g. when no disk is selected, even if there's just one), it will be sufficient to change how the method behaves. No need to touch the tests.

All in all - if we decide to have the method in the class, it needs to be smart utility method, not just dumb macro, that needs "non-trivial" external knowledge to be used. It is true, that the tests now have the behaviour wired in, but if we're going to change it, let's make it better, not just equally bad.

lib/anacondalog.pm
83–96

+1, let's add it with the tests.

main.pm
93

I see your point, and it is definitely valid. I really like the "automagic" for loading tests based on the name, and I understand the concern about copying code around (not necessarily a great idea long-term). How about this:

my $storage;
if (get_var('DISK_CUSTOM')) {
    $storage = get_var('CASEDIR')."/tests/disk_custom_".get_var('DISK_CUSTOM').".pm";
}
else{
    my $disk_guided = get_var('DISK_GUIDED');
    # if DISK_GUIDED is unset, or one of [...]
    if (! $disk_guided || $disk_guided ~~ ['empty', 'free_space']){
        $storage = get_var('CASEDIR')."/tests/disk_guided_empty.pm";
    }
    else{
        $storage = get_var('CASEDIR')."/tests/disk_guided_".$disk_guided.".pm";
    }
}
autotest::loadtest $storage;

That would solve both mine, and yours concerns - it is explicit, smart, can have the post-install scripts selected based on the value of DISK_GUIDED, and won't result in copying code around unnecessarily.

125–137

Fair points. Please just change the -e ... as mentioned in the comments above.

137

Once again, I know that this might just seem like a technicality, but please use:

if ($storagepost) { autotest::loadtest $storagepost }

instead. I know how && and || side-effects work, but i prefer this, more explicit version.

adamwill updated this revision to Diff 1268.Jul 30 2015, 11:15 PM

Revised to accept pretty much all suggestions.

OK, fine, you guys beat me down - select_disks now works as you
requested, it selects exactly the number of disks specified, and 0 is
a valid value (but the default is 1). I haven't actually *tested* the
0 behaviour as we have no tests that use it, but it Should work (S
word alert! S word alert!)

I changed the style of the conditionals you guys didn't like.

jskladan's approach to the $storage thing seems reasonable; if you
write a guided storage test that wants to use disk_empty you'll have
to explicitly add it to the list, but hey, that doesn't seem too
onerous, and I don't think we will have a *huge* number of such tests.
So, I went with it pretty much as he wrote it.

I added the requested comments plus a few more to explain things in
main.pm and anacondalog.pm.

I didn't take out custom_scheme_select yet, but if you're OK with this
otherwise but you really want it to go, just accept it and let me know
and I'll take it out on commit (and bring it back in with the first
test that needs it).

adamwill updated this revision to Diff 1269.Jul 30 2015, 11:57 PM

Couple of small fixes - check NUMDISKS not HDD_2 to find multi-disk
cases, and stick a sleep in before clicking INSTALLATION DESTINATION
because when it happens after software selection it can hit the
animation click fail problem.

garretraziel accepted this revision.Jul 31 2015, 7:23 AM

Ok, this looks good to me.

jskladan accepted this revision.Jul 31 2015, 7:40 AM

Awesome!
Feel free to keep the "unused" methods in this diff, won't make any difference, since the tests that use the methods will probably come just after this diff is merged.

This revision is now accepted and ready to land.Jul 31 2015, 7:40 AM
This revision was automatically updated to reflect the committed changes.