"because I could not find how to switch keymap for Arabic on the login prompt."

This is because there is no console layout for Arabic. At least, not in Fedora. Debian ships one (in its console-data package where its console keymaps live).

I ran into this a few years ago when trying to fix all this switched keyboard layout nonsense (man, that was a fun time).

So, on an Arabic install, in X/Wayland you get switched Arabic/US layouts, but at the console you only get US. You cannot type Arabic at the console.

So, a few notes:

• We use spaces not tabs here!
• You don't really need to specify a 'default' value for get_var usually. So you can simplify if (get_var("LANGUAGE","") ne "arabic") to if (get_var("LANGUAGE") ne "arabic"). It's arguably very slightly sloppy with types, but it's what we do all over the place already anyway (and what SUSE do in their tests).
• I'd prefer the USER_LOGIN and LANGUAGE conditions to be combined rather than nested, I think for two conditions it's cleaner just to combine them than nest them. So something like if (get_var("USER_LOGIN") ne "false" && get_var("LANGUAGE") ne "arabic").
• You should also include a comment explaining why we're not doing this if the language is Arabic. As you may have noticed I like comments everywhere explaining everything. Someone looking at this code who hadn't seen this ticket would have no idea at all why we're mysteriously skipping the user login just in Arabic. That kind of thing should always be explained in a comment.

I haven't looked through the needles yet, I'll try and take a look at those on Monday.

Thanks for the PR!

BTW, I filed an issue on kbd not including an Arabic layout...

Thank you for the peer review.

• I was working in VIM and thought that VIM replaces the tab with spaces. I will have to look onto my settings.
• I used the default value, because Petr told me to do it. I will not use it, if it is not necessary.
• I will fix the ifs and the comments.

Have a nice day.

Using a default value isn't wrong at all, it's just that we don't do it anywhere else and I kinda like to be consistent (otherwise someone looking at the code might think there's a specific need to make the default value "" just here). Now if one of you wanted to submit a PR which changed all the similar code in the project to use default values, we could argue about whether that's technically an improvement! :D

Hum, there's something odd going on now - on the 'Files Changed' tab this PR is shown as including some changes that are from commits I've made to master recently, like the changes on the lib/utils.pm file...but the 'patch' version of the PR doesn't show those changes. Also the 'Commits' tab shows two apparently identical commits by root with the summary "Add test for Arabic installation." Not entirely sure what's happening there...

Also this is not going to work:

get_var("USER_LOGIN") eq "true"

you really need to do what I suggested:

get_var("USER_LOGIN") ne "false"

the two are not equivalent, though at first glance you might think so. USER_LOGIN isn't a true logical boolean, it's just a text string. We don't ever set its value to 'true', in fact. In many tests it's not set at all, which means that the test will create a user called 'test' during install and test user login after install. If it's set to any string other than 'false', then that string is used as the username for the account instead of 'test' - i.e. the normal purpose of the variable is to change the name of the test account. The string 'false' is a magic value meaning "don't create a user during install or try to log in as a user after install at all".

So you really have to check for the magic value 'false' here :)

So I just started looking through the needles on this. One problem immediately hits me: the needle for the tag anaconda_workstation_selected has the filenames package_selection_arabic.{json,png}. This is confusing. Needle filenames should just about always be obviously related to the primary needle tag (so the needle files for a needle intended to be matched on the taganaconda_workstation_selected should probably highly resemble that name). Look at other needles as a reference. In this case, other needles with that tag have filenames like workstation_selected.json, workstation_selected-bgo771127.json...so I'd go with workstation_selected_arabic.json in this case.

The install_lang_continue needle match area is too large. Again, follow the other similar needles as a guide: they match a rectangle entirely inside the 'continue' button. The match in the needle proposed here encompasses the entire Continue button, the space between it and the Quit button, and half of the Quit button. This means the needle might stop matching due to a trivial change in button spacing or button border rendering, neither of which is something we want to break tests. In fact, as the text isn't translated, I'd guess you might as well just add the LANGUAGE-arabic tag to the English button's JSON here, I guess. Saves a needle. Or does that needle not match for some reason? Same thing applies to several other needles for untranslated text.

The install_lang_selected, install_lang_continue and install_lang_filtered needles are all in the wrong directory - they're in needles/anaconda/universal/arabic, but should be in needles/anaconda/lang_select/arabic. That directory appears to contain copies of all the needles from needles/anaconda/lang_select/russian instead, which obviously it shouldn't. They're also not named like the equivalent needles in other languages - we call those install_lang_<langname>_selected(-foo).json rather than install_lang_selected-<langname>(-foo).json. That itself is not consistent with other needles, but, uh...may as well stay...consistently inconsistent...I guess? :P That's not a vital point though. Also, the selected needle should also have the filtered tag; this is useful if the language gets pre-selected for some reason.

The new user_created_arabic needle is wrong. It's meant to match on the username, not the spoke icon (a black spoke icon doesn't tell that a user has been created), and because the username is 'qwerty', a new needle shouldn't be needed at all: one of the existing user_created_qwerty needles should actually match. I see you've added the LANGUAGE-arabic tag to user_created_qwerty-20160825.json, which seems correct; if that does not match it may be because the needles include a bit too much blue space to the right of the 'qwerty' text. I'd advise dropping the new needle and ensuring user_created_qwerty-20160825 matches.

The match area of the root_password_screen_arabic needle is too big, it only needs to match the 'Root password:' text.

The user_creation_screen needle match area is a bad idea. It's not good to match in any of the 'themed' areas in anaconda because they have a tendency to change between releases and images. That's why the other needles for this tag match on text elsewhere in the screen. I'd suggest the translated 'first and last name' text entry box title - the bold text above the bold "User name" text.

The anaconda_error_arabic needle seems entirely bogus. That needle is meant to match the "An error has occurred" dialog that pops up when anaconda crashes, not the orange warning bar. I can't figure out why you included this needle, but it's certainly wrong. The needle is only used in the post-fail hook for anaconda tests, where if that screen is showing, we try to upload an anaconda traceback.

The user_creation_arabic needle seems entirely unnecessary, the existing user_creation needle should match fine. Does it not?

layout_ascii_arabic should be called layout_us_arabic (or, possibly, layout_us_rtl, in fact, as this isn't really arabic-specific, it's right-to-left language specific). Same applies to all the other needles for "US / en layout with right-to-left rendering" needles - call them us_arabic or us_rtl not ascii_arabic. Also, rather than gnome for the GDM layout needles and gnome_de for the GNOME overview layout needles, call them gdm and overview, that's what we do in other languages (see e.g. layout_us-gdm and layout_us-overview).

graphical_login_input_arabic should be called login_gdm_input_arabic, and should match on the text input box title and the box itself like the equivalent needles for other languages.

That's all the notes I have from my first look through the needles - thanks!

Hello Adam,
I get your point. As I am in the state of "tabula rasa" for any OpenQA
work, needles included, I guess I will probably have to hit my nose
somewhere to see what the needles should look like ideally. I will try to
revise them according to your suggestions though.
As for the unnecessary needles, the test was stopping at certain locations,
so I added a needle to cover that blind spot. I thought that this approach
could not create unnecessary needles, because if the needles were there,
the test would not have stopped. Am I mistaken?

On Fri, May 18, 2018 at 2:34 AM, Adam Williamson pagure@pagure.io wrote:

adamwill commented on the pull-request: Add test for Arabic installation= , fixes #76. that you are following:
` So I just started looking through the needles on this. One problem immediately hits me: the needle for the taganaconda_workstation_selecte=
dhas the filenamespackage_selection_arabic.{json,png}. This is confusing. Needle filenames should just about always be obviously related to the primary needle tag (so the needle files for a needle intended to b= e matched on the taganaconda_workstation_selectedshould probably highly resemble that name). Look at other needles as a reference. In this case, other needles with that tag have filenames likeworkstation_selected.json,workstation_selected-bgo771127.json...so I'd go withworkstation_selected_arabic.json` in this case.

The install_lang_continue needle match area is too large. Again, follow
the other similar needles as a guide: they match a rectangle entirely
inside the 'continue' button. The match in the needle proposed here
encompasses the entire Continue button, the space between it and the Quit
button, and half of the Quit button. This means the needle might stop
matching due to a trivial change in button spacing or button border
rendering, neither of which is something we want to break tests. In fact,
as the text isn't translated, I'd guess you might as well just add the
LANGUAGE-arabic tag to the English button's JSON here, I guess. Saves=
a
needle. Or does that needle not match for some reason? Same thing applies
to several other needles for untranslated text.

The install_lang_selected, install_lang_continue and
install_lang_filtered needles are all in the wrong directory - they're =
in
needles/anaconda/universal/arabic, but should be in
needles/anaconda/lang_select/arabic. That directory appears to contain
copies of all the needles from needles/anaconda/lang_select/russian
instead, which obviously it shouldn't. They're also not named like the
equivalent needles in other languages - we call those
install_lang_<langname>_selected(-foo).json rather than
install_lang_selected-<langname>(-foo).json. That itself is not
consistent with other needles, but, uh...may as well stay...consistently
inconsistent...I guess? :P That's not a vital point though. Also, the
selected needle should also have the filtered tag; this is useful if
the language gets pre-selected for some reason.

The new user_created_arabic needle is wrong. It's meant to match on the
username, not the spoke icon (a black spoke icon doesn't tell that a user
has been created), and because the username is 'qwerty', a new needle
shouldn't be needed at all: one of the existing user_created_qwerty needl=
es
should actually match. I see you've added the LANGUAGE-arabic tag to user_created_qwerty-20160825.json, which seems correct; if that does not
match it may be because the needles include a bit too much blue space to
the right of the 'qwerty' text. I'd advise dropping the new needle and
ensuring user_created_qwerty-20160825 matches.

The match area of the root_password_screen_arabic needle is too big, it
only needs to match the 'Root password:' text.

The user_creation_screen needle match area is a bad idea. It's not good
to match in any of the 'themed' areas in anaconda because they have a
tendency to change between releases and images. That's why the other
needles for this tag match on text elsewhere in the screen. I'd suggest t=
he
translated 'first and last name' text entry box title - the bold text abo=
ve
the bold "User name" text.

The anaconda_error_arabic needle seems entirely bogus. That needle is
meant to match the "An error has occurred" dialog that pops up when
anaconda crashes, not the orange warning bar. I can't figure out why you
included this needle, but it's certainly wrong. The needle is only used
in the post-fail hook for anaconda tests, where if that screen is showing=
,
we try to upload an anaconda traceback.

The user_creation_arabic needle seems entirely unnecessary, the existin=
g
user_creation needle should match fine. Does it not?

layout_ascii_arabic should be called layout_us_arabic (or, possibly,
layout_us_rtl, in fact, as this isn't really arabic-specific, it's
right-to-left language specific). Same applies to all the other needles
for "US / en layout with right-to-left rendering" needles - call them
us_arabic or us_rtl not ascii_arabic. Also, rather than gnome for
the GDM layout needles and gnome_de for the GNOME overview layout
needles, call them gdm and overview, that's what we do in other
languages (see e.g. layout_us-gdm and layout_us-overview).

graphical_login_input_arabic should be called login_gdm_input_arabic,
and should match on the text input box title and the box itself like the
equivalent needles for other languages.

That's all the notes I have from my first look through the needles -
thanks!
``

To reply, visit the link below or just reply to this email
https://pagure.io/fedora-qa/os-autoinst-distri-fedora/pull-request/77

--=20

Luk=C3=A1=C5=A1 R=C5=AF=C5=BEi=C4=8Dka

FEDORA QE, RHCE

Red Hat

https://www.redhat.com

Purky=C5=88ova 115

612 45 Brno - Kr=C3=A1lovo Pole

lruzicka@redhat.com
TRIED AND PERSONALLY TESTED, ERGO TRUSTED. https://redhat.com/trusted

"As for the unnecessary needles, the test was stopping at certain locations,
so I added a needle to cover that blind spot. I thought that this approach
could not create unnecessary needles, because if the needles were there,
the test would not have stopped. Am I mistaken?"

No, you're not mistaken, but if an existing needle clearly should have matched, it'd be good to investigate and try if possible to figure out why it didn't, and maybe adjust it to match if that's possible. So, view this as a request to check each case where you had to add a new needle that matched untranslated text, and figure out why the existing needles didn't match at that point :)

Hum - it seems pagure doesn't send notifications for 'new commit added to existing pull request', at least not by default, so I only just noticed this. Will review ASAP. @pingou is that possibly a Pagure issue?

Sorry, didn't get to this today, will try and do it tomorrow. @tibbs nerd-sniped me into spending all day fixing rpmgrill instead...

OK, so looked over the new commit quickly. So, here's the thing: my big thing is consistency. Whatever you're doing, do it the same way it was done before. If you think the way it was done before is wrong, don't just do this one thing a different way: either get all the other things changed first then do the new thing the same way, or do the new thing the old way then change everything over to a new way later.

What I'm talking about specifically is the needle names/paths like this:

needles/anaconda/install_process/arabic/anaconda_install_root_password_screen_arabic.json

it's not an objectively bad or wrong name, not like before where you had a couple of needles with names that just had nothing to do with the tag at all - but it's not the same as the other languages. There's a model to follow here, and for a first pull request, it'd really be good to just follow the model. The equivalent needles for French are needles/anaconda/install_process/french/root_password_screen_french-20160825.json and needles/anaconda/install_process/french/root_password_screen_french-cantarell101.json , the equivalents for Russian are needles/anaconda/install_process/russian/root_password_screen_russian-20160825.json and needles/anaconda/install_process/russian/root_password_screen_russian-cantarell101.json, the equivalents for Japanese are needles/anaconda/install_process/japanese/root_password_screen_japanese-20161217.json and needles/anaconda/install_process/japanese/root_password_screen_japanese-cantarell101.json.

The existing naming scheme is clear: this needle should be called needles/anaconda/install_process/arabic/root_password_screen_arabic.json. Just as it was before the rename commit, in fact. We can discuss whether the current directory layout and naming scheme is the best choice, and we can change it. Maybe there is a better way to do it - but having just this one set of needles doing it a different way from all the other obviously similar needles isn't the way to go about that. In this PR I'm just trying to get you to add this new test in a way as close as possible to the existing conventions. I didn't suggesting renaming all the needles to include the full tag name in the initial review, so I'm not sure why you did that.

The commit does make some of the changes I suggested, so that part's great. But please revert the changes I didn't ask for :) thanks!

also, of course, there's some stuff I mentioned which wasn't covered in that commit, so we still need that.

Yeah, I will try to get it correct. I do not know, what is a better way. I
only need to get used to some kind of system, because I cannot rely on my
memory. That means that I will forget the contexts quickly when it is not
self explanatory. The names, I used, where suggested by the OpenQA system
and had the tag name involved which I found clever.
Luk.

On Fri, May 25, 2018 at 11:06 PM, Adam Williamson pagure@pagure.io wrote:

adamwill commented on the pull-request: Add test for Arabic installation= , fixes #76. that you are following:
``
OK, so looked over the new commit quickly. So, here's the thing: my big
thing is consistency. Whatever you're doing, do it the same way it was
done before. If you think the way it was done before is wrong, don't just
do this one thing a different way: either get all the other things change=
d
first then do the new thing the same way, or do the new thing the old way
then change everything over to a new way later.

What I'm talking about specifically is the needle names/paths like this:

needles/anaconda/install_process/arabic/anaconda_ install_root_password_screen_arabic.json

it's not an objectively bad or wrong name, not like before where you had =
a
couple of needles with names that just had nothing to do with the tag at
all - but it's not the same as the other languages. There's a model to
follow here, and for a first pull request, it'd really be good to just
follow the model. The equivalent needles for French are
needles/anaconda/install_process/french/root_password_screen_french-2016= 0825.json
and needles/anaconda/install_process/french/root_password_ screen_french-cantarell101.json , the equivalents for Russian are
needles/anaconda/install_process/russian/root_password_screen_russian-20= 160825.json
and needles/anaconda/install_process/russian/root_password_ screen_russian-cantarell101.json, the equivalents for Japanese are
needles/anaconda/install_process/japanese/root_password_screen_japanese-= 20161217.json
and needles/anaconda/install_process/japanese/root_ password_screen_japanese-cantarell101.json.

The existing naming scheme is clear: this needle should be called
needles/anaconda/install_process/arabic/root_password_screen_arabic.json=.
Just as it was before the rename commit, in fact. We can discuss whethe=
r
the current directory layout and naming scheme is the best choice, and we
can change it. Maybe there is a better way to do it - but having just
this one set of needles doing it a different way from all the other
obviously similar needles isn't the way to go about that. In this PR I'm
just trying to get you to add this new test in a way as close as possible
to the existing conventions. I didn't suggesting renaming all the needles
to include the full tag name in the initial review, so I'm not sure why
you did that.

The commit does make some of the changes I suggested, so that part's
great. But please revert the changes I didn't ask for :) thanks!

also, of course, there's some stuff I mentioned which wasn't covered in
that commit, so we still need that.
``

To reply, visit the link below or just reply to this email
https://pagure.io/fedora-qa/os-autoinst-distri-fedora/pull-request/77

--=20

Luk=C3=A1=C5=A1 R=C5=AF=C5=BEi=C4=8Dka

FEDORA QE, RHCE

Red Hat

https://www.redhat.com

Purky=C5=88ova 115

612 45 Brno - Kr=C3=A1lovo Pole

lruzicka@redhat.com
TRIED AND PERSONALLY TESTED, ERGO TRUSTED. https://redhat.com/trusted

@lruzicka the way openQA names the needles for you is sort of based on all the needles being in a single big directory, which is how SUSE does it and how we used to do it. When @jskladan came up with the plan to split our needle repo into subdirectories he basically created the subdirectory names from common needle tag prefixes, so if you include the full tag in the needle filename too it's essentially redundant - you have anaconda/install_process in the path name, and anaconda_install in the filename.

Personally I would actually probably favour ditching the subdirectory splits and going back to all needles in a single directory; I don't think the subdirectory split really gains us anything much and it causes various annoyances like this "what directory should this needle go in" dance. But that's outside the scope of this PR - if we're going to do that, we should make it a separate PR.

Why do the last three commits in the series make the JSON files into one-liners? I mean, it's not necessarily a problem, just seems slightly odd. Is that what this new tool you wrote does?

OK, so, this is looking better now. One requested rename is still missing - the layout needles, you used a different naming scheme from all the other languages. For other languages we used layout_(langname)-gdm as the base of the name for the GDM needles, and layout_(langname-overview as the base of the name for the needles that match on the overview screen once logged into GNOME. You used gnome_de and gnome in your filenames. Also, that ascii_arabic name is just wrong; those match on 'us'. As mentioned we should call them something like us_ltr or so (so layout_us_ltr-gdm for instance).

I think I figured out why we have this issue of existing needles not matching when they should: it's to do with the language filtering trick in main.pm. Look at unregister_prefix_tags and cleanup_needles in that file: I came up with this mechanism where, when doing a language-specific test, needles for other languages are un-registered. This is intended to make sure the installer is translated (so e.g. the French install test doesn't pass by matching English needles the whole way, if the installer isn't translated).

This mechanism kinda assumes languages have complete translations, though. So I'd suggest what we should do is throw away all the duplicate needles and add LANGUAGE-arabic tags to all the English needles we actually want to match for the Arabic test (all the cases where we know the text is untranslated, there are no LTR issues, and so the English needle should match). So I'd throw away spoke_done_arabic and add LANGUAGE-arabic tags to all the English spoke_done needles, for e.g. Same for begin_installation, install_lang_continue, user_created (add the tag to the user_created_qwerty needles), install_done-reboot, and getting_started. Does that make sense? Thanks!

Sorry for the long review on this one :)