New tests and changes to handle YaST's new libstorage #3409

cwh42 · 2017-08-14T16:01:54Z

No description provided.

okurz · 2017-08-14T19:19:12Z

partitioning_guided.pm

+# SUSE's openQA tests
+#
+# Copyright © 2009-2013 Bernhard M. Wiedemann
+# Copyright © 2012-2016 SUSE LLC


Please only use current year

okurz · 2017-08-14T19:19:22Z

partitioning_guided.pm

+# without any warranty.
+
+# Summary: Use guided partitioning setup to delete an existing windows partition. 
+# Maintainer: cwh


Can you use an email instead?

okurz · 2017-08-14T19:19:40Z

partitioning_guided.pm

+
+
+use warnings;
+use parent qw(y2logsstep);


Please use single ticks for single value imports

okurz · 2017-08-14T19:20:15Z

partitioning_guided.pm

+    # Enter guided setup
+    send_key "alt-g";
+
+    assert_screen('inst-partition-guided', 10);


Just assert_screen with default timeout should be fine

okurz · 2017-08-14T19:20:41Z

partitioning_guided.pm

+}
+
+sub run() {
+    my $self             = shift;


Just delete if you don't use it

okurz · 2017-08-14T19:22:37Z

tests/installation/installation_overview.pm

+        assert_screen "inst-overview-booting";
+
+        # preserve it for the video
+        wait_idle 10;


okurz · 2017-08-14T19:23:14Z

tests/installation/partitioning_expert.pm

+# without any warranty.
+#
+#
+# Summary: 


That's not a very convincing summary ;-)

okurz · 2017-08-14T19:24:31Z

tests/installation/partitioning_expert.pm

+    assert_and_click 'hard-disks';
+    assert_and_click 'home';
+
+    send_key 'alt-d'; # Delete


I guess this might fail because the next keypress might get lost. Better guard it with wait_screen_change -> wait_screen_change { send_key 'alt-d' };

okurz · 2017-08-14T19:25:22Z

tests/installation/partitioning_expert.pm

+    save_screenshot;
+
+    send_key $cmd{accept};
+    die "/home still there" if check_screen('home');


Please don't use check_screen with default timeout. Either surround the previous send_key with wait_screen_change or use a multi-tag assert_screen plus match_has_tag

okurz · 2017-08-14T19:26:19Z

tests/installation/partitioning_lvm.pm

+    my $numdisks = get_var("NUMDISKS");
+    print "NUMDISKS = $numdisks\n";
+
+    check_screen([qw(inst-partition-radio-buttons inst-partition-guided inst-partitioning-scheme)], 10);


Please don't use check_screen with timeout other than 0. Either surround the previous send_key with wait_screen_change or use a multi-tag assert_screen plus match_has_tag

I just extended the existing code, here. So that check_screen …, 10; already was there, before.
Maybe it is worth rewriting the old code anyway?

okurz · 2017-08-14T19:27:24Z

Delete dot at end-of-line of commit message subject lines and PR messages

rwx788 · 2017-08-15T07:19:41Z

tests/installation/installation_overview.pm

@@ -32,6 +32,21 @@ sub run {
        assert_screen "inst-xen-pattern";
    }

+    # preserve it for the video
+    wait_idle 10;


You can use wait_still_screen instead.

rwx788 · 2017-08-15T07:24:26Z

tests/installation/partitioning_lvm.pm

+    elsif (get_var("STORAGE_NG")) {
+        if ($numdisks <= 1) {
+            die "Guided workflow does not skip disk selection when only one disk!"
+                if !match_has_tag('inst-partitioning-scheme');


Matter of taste, but it's more wordy to use "unless" here and get rid of negation

For my personal taste actually an "if not" is more obvious than a "unless" – but if ther is any convention about this, I will of course change it.

Nope, that's why I say that it's matter of taste, so no change is really required. Only point that negated expressions usually are evaluated a little slower, but probably unless does it anyway underneath.

just use "unless" :-)

Acually, when reading Damian Conway's "Perl Best Practices" I learned, that using negative modifiers like unless and until should be avoided as much as possible. See #13 in http://www.perlmonks.org/?node_id=989115

It's all subjective, but I totally agree with subpoints: Use unless and until only with positive statements so you don't create a double negative logic, which would be twice as confusing! Never follow an unless block with an else block.
If it's used in that way that you can read it in plain English, like "die unless" it's more wordy than "die if not", which is totally perlish and is confusing for some people. I'm fine if we are just consistent, so I do not request a change here. And thanks for the link, it's nice article!

rwx788 · 2017-08-15T07:24:48Z

tests/installation/partitioning_lvm.pm

+        }
+        else {
+            die "Guided workflow does skip disk selection when more than one disk! (NUMDISKS=$numdisks)"
+                if !match_has_tag('inst-partition-guided');


Same as above.

rwx788 · 2017-08-15T07:26:38Z

tests/installation/partitioning_lvm.pm

+
+            if (!get_var('ENCRYPT_ACTIVATE_EXISTING')) {
+                assert_screen 'inst-encrypt-password-prompt';
+                #send_key 'tab'; # might be needed again later 


Probably we can remove this one

rwx788 · 2017-08-15T07:36:08Z

tests/installation/partitioning_lvm.pm

+                #send_key 'tab'; # might be needed again later 
+                type_password;
+                send_key 'tab';
+                type_password;


Here is a little tricky part, as what we have seen sometimes that we press keys to fast and for example start typing password before we got into password field. But let's see if we experience this issue.

cwh42 · 2017-08-15T13:28:26Z

Integrated your proposed changes

rwx788 · 2017-08-16T11:20:51Z

tests/installation/grub_test.pm

+
+    if (get_var("STORAGE_NG") && get_var("ENCRYPT")) {
+        my @tags = ();
+        for ( my $disk = 0; $disk < get_var("NUMDISKS", 1); $disk++ ) {


Travis complains about spaces after and before brackets.

cwh42 · 2017-08-16T15:14:45Z

The setup of perlcritic here in travis is different than when run locally on my and Rodion's machine.
I wasted half a day of debugging here until I found the problem because the error message contains a line number but not the file where the error is found.

okurz · 2017-08-17T11:49:24Z

On Wednesday, 16 August 2017 08:14:46 CEST Christopher Hofmann wrote: The setup of perlcritic here in travis is different than when run locally on my and Rodion's. Also it does not tell the file, where it found the error. So the problem here is unfixable for me. I wasted half a day of debugging here and will be on vacation from now on. Contact my team if you want this to be merged before week 35 when I am back.

It seems you ran perltidy locally but it failed to read the os-autoinst based .perltidyrc so make sure you have an existing "os-autoinst" linked into os-autoinst- distri-opensuse or call `make prepare`

rwx788 · 2017-08-17T12:30:39Z

@okurz We did exactly this and it looks like it uses different profile. In the end main point here is that it's not clear about which file travis is complaining about, otherwise it's ok to fix results after pushing the commit.

okurz · 2017-08-17T14:57:35Z

squashed and merged locally, approved by @rwx788

cwh42 added 2 commits August 14, 2017 17:54

Use the new guided partitioning workflow for Storage NG

20904e9

Handle grub password prompt for crypted disk.

8616fa8

okurz reviewed Aug 14, 2017

View reviewed changes

rwx788 reviewed Aug 15, 2017

View reviewed changes

cwh42 added 5 commits August 15, 2017 15:26

Cope with the new crypted volume activation dialog

64ed6db

Use new module 'partitioning_guided' to delete windows partitions

b677dc6

Scroll down in inst-overview to show errors

757368b

Confirm installation to be able to see the proposal in the test

5aff330

New test for expert partitioner

fcd5d79

cwh42 force-pushed the master branch from 2ffa28b to cd91806 Compare August 15, 2017 13:27

cwh42 changed the title ~~New tests and changes to handle YaST's new libstorage.~~ New tests and changes to handle YaST's new libstorage Aug 15, 2017

rwx788 reviewed Aug 16, 2017

View reviewed changes

cwh42 force-pushed the master branch 3 times, most recently from 3909015 to a658f87 Compare August 16, 2017 14:54

Fix issues found during review

1df435d

cwh42 force-pushed the master branch from a658f87 to 1df435d Compare August 16, 2017 15:32

Extend expert partitioner test

3c5751d

okurz closed this Aug 17, 2017

ancorgs mentioned this pull request Aug 18, 2017

Needles for storage-ng os-autoinst/os-autoinst-needles-opensuse#252

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New tests and changes to handle YaST's new libstorage #3409

New tests and changes to handle YaST's new libstorage #3409

cwh42 commented Aug 14, 2017

okurz Aug 14, 2017

okurz Aug 14, 2017

okurz Aug 14, 2017

okurz Aug 14, 2017

okurz Aug 14, 2017

okurz Aug 14, 2017

okurz Aug 14, 2017

okurz Aug 14, 2017

okurz Aug 14, 2017

okurz Aug 14, 2017

cwh42 Aug 15, 2017

okurz Aug 15, 2017

okurz commented Aug 14, 2017

rwx788 Aug 15, 2017

rwx788 Aug 15, 2017

cwh42 Aug 15, 2017

rwx788 Aug 15, 2017

okurz Aug 15, 2017

cwh42 Aug 16, 2017 •

edited

Loading

rwx788 Aug 16, 2017

rwx788 Aug 15, 2017

rwx788 Aug 15, 2017

rwx788 Aug 15, 2017

cwh42 commented Aug 15, 2017

rwx788 Aug 16, 2017

cwh42 commented Aug 16, 2017 •

edited

Loading

okurz commented Aug 17, 2017 via email

rwx788 commented Aug 17, 2017

okurz commented Aug 17, 2017

New tests and changes to handle YaST's new libstorage #3409

New tests and changes to handle YaST's new libstorage #3409

Conversation

cwh42 commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okurz commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwh42 Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwh42 commented Aug 15, 2017

Choose a reason for hiding this comment

cwh42 commented Aug 16, 2017 • edited Loading

okurz commented Aug 17, 2017 via email

rwx788 commented Aug 17, 2017

okurz commented Aug 17, 2017

cwh42 Aug 16, 2017 •

edited

Loading

cwh42 commented Aug 16, 2017 •

edited

Loading