Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Select yast development pattern only #15693

Merged
merged 1 commit into from Oct 27, 2022
Merged

Select yast development pattern only #15693

merged 1 commit into from Oct 27, 2022

Conversation

lemon-suse
Copy link
Contributor

@lemon-suse lemon-suse commented Oct 13, 2022 •
edited

@lemon-suse lemon-suse force-pushed the select_pattern_only_yast_development branch 5 times, most recently from df80d41 to b993499 Compare October 13, 2022 06:21
@jknphy jknphy added the qe-yam label Oct 13, 2022
@coolgw coolgw requested a review from ge0r October 19, 2022 02:08
@jknphy
Copy link
Contributor

jknphy commented Oct 20, 2022 •
edited

@lemon-suse could we apply solution in #15675 here ?
instead of a hack and hard to understand code in that PR we find useful to create specific test module that does ONE thing.
There, creating new test module tests/installation/select_visible_patterns_from_bottom.pm (notice I remomed 'only' from existing name to reference that there is not clean up of all patterns) it should do, you just need to create similar function that that one, but without the clean up of the whole list.

For patterns we will not need PATTERNS | default,yast-development just 'yast-development' I guess.

The result should be clean and simple. We can refactor those in the future and send the key to locate by test data, but for now two test modules fits well.wdyt?

@lemon-suse lemon-suse force-pushed the select_pattern_only_yast_development branch 4 times, most recently from 162846c to 8acc6eb Compare October 21, 2022 04:52
@lemon-suse
Copy link
Contributor Author

@lemon-suse could we apply solution in #15675 here ? instead of a hack and hard to understand code in that PR we find useful to create specific test module that does ONE thing. There, creating new test module tests/installation/select_visible_patterns_from_bottom.pm (notice I remomed 'only' from existing name to reference that there is not clean up of all patterns) it should do, you just need to create similar function that that one, but without the clean up of the whole list.

For patterns we will not need PATTERNS | default,yast-development just 'yast-development' I guess.

The result should be clean and simple. We can refactor those in the future and send the key to locate by test data, but for now two test modules fits well.wdyt?

Ok, updated. The new VR: http://openqa.nue.suse.com/tests/9775751#step/select_visible_patterns_from_bottom/12

@lemon-suse lemon-suse force-pushed the select_pattern_only_yast_development branch 5 times, most recently from e233505 to e7b098c Compare October 24, 2022 01:08
@lemon-suse lemon-suse force-pushed the select_pattern_only_yast_development branch 7 times, most recently from 91f7c31 to 89680e4 Compare October 27, 2022 08:27
@lemon-suse lemon-suse changed the title Select yast development pattern only [WIP]Select yast development pattern only Oct 27, 2022
@lemon-suse lemon-suse changed the title [WIP]Select yast development pattern only Select yast development pattern only Oct 27, 2022
@lemon-suse lemon-suse force-pushed the select_pattern_only_yast_development branch from 89680e4 to 11e0daa Compare October 27, 2022 09:19
@lemon-suse lemon-suse changed the title Select yast development pattern only [WIP]Select yast development pattern only Oct 27, 2022
@lemon-suse lemon-suse force-pushed the select_pattern_only_yast_development branch from 11e0daa to e524dcd Compare October 27, 2022 11:48
@lemon-suse lemon-suse changed the title [WIP]Select yast development pattern only Select yast development pattern only Oct 27, 2022
@JRivrain JRivrain merged commit c56779d into os-autoinst:master Oct 27, 2022
jknphy
jknphy reviewed Oct 31, 2022
View reviewed changes
tests/installation/select_visible_patterns_from_bottom.pm
$self->go_to_patterns();
my @patterns = grep($_, split(/,/, get_required_var('PATTERNS')));
# delete special 'default' key from the check
@patterns = grep { $_ ne 'default' } @patterns;
Copy link
Contributor

@jknphy jknphy Oct 31, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this line, we should remove 'default' from settings via MR.

Copy link
Contributor

@JRivrain JRivrain Nov 2, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically yes, however someone checking the settings can interpret that "PATTERNS=yast_develpoment" may mean only yast development, and not default patterns, as all other tests using that variable specify it otherwise.

Copy link
Contributor

@jknphy jknphy Nov 2, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our way is to check the name of the test module, if we pass as settings or test data is just by our own convenience, we have tests/installation/select_only_visible_patterns_from_top.pm for disambiguation with only in the name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jknphy jknphy jknphy left review comments

@JRivrain JRivrain JRivrain approved these changes

@ge0r ge0r ge0r approved these changes

Assignees
No one assigned
Labels
qe-yam
Projects
None yet
Milestone
No milestone
4 participants
@lemon-suse @jknphy @ge0r @JRivrain