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.

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

[WIP] Rancher UI selenium test #11863

Closed
wants to merge 1 commit into from

Conversation

ilausuch
Copy link
Contributor

@ilausuch ilausuch commented Jan 28, 2021

This test checks some initial pages of the rancher UI.
This differs from #11807 because uses Selenium instead of traditional openQA click commands. This difference allows to check the individual pages using the DOM instead of using needles. There are several benefits in this way:

  • Not adding more needles to the needles repository in this case is not necessary.
  • In case some page text changes, for instance in the main page, where a "What's new" dialog appears, we don't need to redo a new needle every time this dialog changes.
  • Using DOM we could accelerate the test, because we can check if one element is visible or not, preventing long sleeps.

Test: https://openqa.opensuse.org/tests/1607472#

See: https://progress.opensuse.org/issues/88183

@ilausuch ilausuch force-pushed the rancher_ui_test branch 3 times, most recently from de2cf13 to b218735 Compare January 28, 2021 13:07
@ilausuch ilausuch requested a review from ge0r January 28, 2021 15:16
@pdostal
Copy link
Member

pdostal commented Jan 29, 2021

Do you Ivan think that it would be good approach to create a cluster using Selenium?

@ilausuch
Copy link
Contributor Author

ilausuch commented Feb 4, 2021

Do you Ivan think that it would be good approach to create a cluster using Selenium?

Yes of course (but with #11807 too). The big question here, is what cloud provider we are going to use, or we are going to use bare-metal.

I think the creation of this extended test would exceed the objective of this PR.

@pdostal
Copy link
Member

pdostal commented Feb 4, 2021

Do you Ivan think that it would be good approach to create a cluster using Selenium?

Yes of course (but with #11807 too). The big question here, is what cloud provider we are going to use, or we are going to use bare-metal.

I think the creation of this extended test would exceed the objective of this PR.

I plan to do this over the weekend and I will use multi-machine tests.
Each running test is one QEMU VM so no provider is needed.

Copy link
Member

@ge0r ge0r left a comment

Choose a reason for hiding this comment

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

Perfect! Nice kick start for selenium testing Ivan!

@ilausuch ilausuch force-pushed the rancher_ui_test branch 2 times, most recently from 3c216d2 to 2ae61dd Compare February 4, 2021 11:58
Copy link
Member

@pdostal pdostal left a comment

Choose a reason for hiding this comment

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

Good! I have just one suggestion :)

@ilausuch ilausuch force-pushed the rancher_ui_test branch 2 times, most recently from 344e936 to 0c79c9f Compare February 4, 2021 16:14
data/rancher/ui_test.py Outdated Show resolved Hide resolved
@ilausuch
Copy link
Contributor Author

ilausuch commented Feb 11, 2021

Last test https://openqa.opensuse.org/tests/1627261#step/rancher_ui_selenium/15 (failed)

driver.find_element_by_xpath("//button[@type='submit']").click()

# Initial page: set the URL
time.sleep(2)
Copy link
Member

Choose a reason for hiding this comment

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

You are using sleep() or are simulating a sleep-like functionality. This is introducing the risk of either unnecessarily wasting time or just shifting race conditions and making them even harder to investigate. You can use sleep but there are only very limited cases where is the best solution (e.g. yield threads in low-level system code). Here you have the classical problem in software design of Synchronization . sleep should not be used in place of proper synchronization methods. https://blogs.msmvps.com/peterritchie/2007/04/26/thread-sleep-is-a-sign-of-a-poorly-designed-program/ is one of multiple references that explains this: "The original problem is likely a timing/synchronization issue, ignoring it by hiding it with Thread.Sleep is only going to delay the problem and make it occur in random, hard to reproduce ways." And if the problem does not occur at least time is wasted.

We are waiting because some condition changes some time ... keyword(s) is/are some time. It is important to know the exact condition that the code should wait for and check for that instead of delaying program execution for an arbitrary amount of time. For openQA tests in general just try to do what you as a human user would also do. You would not look on your watch waiting for exactly 100s before you look at the screen again, right? :)

Same applies for other uses of sleep

See https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/CONTRIBUTING.md#coding-style for more details

x11_start_program('xterm');

type_string "python3 ui_test.py && touch /tmp/ok\n";
assert_script_run("[ -f /tmp/ok ]");
Copy link
Member

Choose a reason for hiding this comment

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

Instead of [...] better use

Suggested change
assert_script_run("[ -f /tmp/ok ]");
assert_script_run('test -f /tmp/ok');

Comment on lines +21 to +23
my ($self) = @_;

my ($self) = @_;
Copy link
Member

Choose a reason for hiding this comment

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

...

$self->select_serial_terminal;

assert_script_run("pip3 install selenium");
assert_script_run('curl -O ' . data_url("rancher/ui_test.py"), timeout => 300);
Copy link
Member

Choose a reason for hiding this comment

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

please delete the w/s at EOL

@kalikiana
Copy link
Member

Closing this as per discussions since this is just a proof of concept at this point.

@kalikiana kalikiana closed this Feb 15, 2021
@pdostal
Copy link
Member

pdostal commented Feb 15, 2021

Closing this as per discussions since this is just a proof of concept at this point.

I'm sorry but I don't understand. @kalikiana did you speak with @ilausuch about this?

@kalikiana
Copy link
Member

kalikiana commented Feb 15, 2021

Closing this as per discussions since this is just a proof of concept at this point.

I'm sorry but I don't understand. @kalikiana did you speak with @ilausuch about this?

Yes. See also poo#88183#note-10. I assume a follow-up conversation will be organized soon to answer questions like where tests live, if Python-Selenium is the right choice etc (afair Perl was also suggested before) and then whatever follows from that will be tested in openQA.

@ilausuch
Copy link
Contributor Author

This is a proof of concept to check if we can use selenium to check the rancher UI and other people is involved and interested in that.

So, I understand that we could close the progress progress ticket because of the implications @kalikiana mentioned. But I think is not a good idea to close the PR because it is still in progress.

For this reason I have to reopen this.

@ilausuch ilausuch reopened this Feb 17, 2021
@okurz
Copy link
Member

okurz commented Feb 17, 2021

So, I understand that we could close the progress progress ticket because of the implications @kalikiana mentioned. But I think is not a good idea to close the PR because it is still in progress.

For this reason I have to reopen this.

There is no point in keeping the PR open for a closed ticket. If you want this PR to be merged then there should be an open ticket covering that. On the other side, a closed PR will still stay as a reference that can be used.

@ilausuch
Copy link
Contributor Author

Closing this, is covered on #11807

@ilausuch ilausuch closed this Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants