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

Replace start_firefox as x11_start_program for firefox_nss #7102

Merged
merged 1 commit into from Mar 21, 2019

Conversation

jouyingbin
Copy link
Contributor

@jouyingbin jouyingbin commented Mar 20, 2019

  1. To avoid start_firefox cause the URL render error randomly (poo#47018)
  2. Provide match_timeout => 360
  3. Add Turn off screensaver before launching firefox

@kravciak kravciak requested a review from dzedro March 20, 2019 09:13
Copy link
Contributor

@lilyeyes lilyeyes left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix.

@dzedro
Copy link
Contributor

dzedro commented Mar 20, 2019

@jouyingbin I have doubts that longer timeout will help, I don't have problem with waiting longer, it's still better to see timeout page in firefox than openqa failure because of short timeout. But if you check the failure last needle failed with black screen, I guess screensaver/blankscreen was activated.

Copy link
Contributor

@dzedro dzedro left a comment

Choose a reason for hiding this comment

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

Timeout in start_firefox can be increased too, but it won't help when screensaver get activated in between

@jouyingbin
Copy link
Contributor Author

@dzedro Thanks for reviewing,
We met the problem before randomly, somemtimes it was passed. So I don't know if it is the timeout is too short. There are some different testing results.
https://openqa.suse.de/tests/2536224#step/firefox_nss/8 [Fail]
https://openqa.suse.de/tests/2510354#step/firefox_nss/5 [Fail]
https://openqa.suse.de/tests/2494389#step/firefox_nss/5 [Passed]

Do you mean that we still use start_firefox with longer timeout?

@dzedro
Copy link
Contributor

dzedro commented Mar 20, 2019

@jouyingbin yes, I think with firefox is longer timeout generally not bad, it's better to see firefox error page than openqa timeout failure, then you have some clue if page timed out or there was another problem, but mostly it's network problem.

@dzedro
Copy link
Contributor

dzedro commented Mar 20, 2019

With timeout above 300, I think it's default screensaver timeout, screensaver will have to be turned off or changed timeout

@jouyingbin
Copy link
Contributor Author

@dzedro Thanks for your suggestions, as in my local system, it takes over 300, thus I set it as 360 for my testing, it really depends on the different network environment, I think we can merge this one and adjust screensaver in the future, thanks.

@jouyingbin
Copy link
Contributor Author

jouyingbin commented Mar 21, 2019

And I also added to turn off gnome screensaver before launching the firefox. [Done]

1. To avoid start_firefox cause the URL render error randomly (poo#47018)
2. Provide match_timeout => 360
3. Add Turn off screensaver before launching firefox
@@ -12,11 +12,13 @@
# Summary: FIPS mozilla-nss test for firefox
# Maintainer: mitiao <mitiao@gmail.com>,
# wnereiz <wnereiz@fsf.member.org>
# Tag: poo#47018
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the "poo#47018" in "Tag" is not the original poo when this test case was added.
And, add another Tag tc#$testcase_number is better if it exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the tc# is not that mandatory for the openQA test case, because the case is slightly different from our original tc#. Furthermore, the test case number will be outdated in the next product SPx release. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK! thanks for the explanation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM

@dzedro dzedro merged commit 26634c7 into os-autoinst:master Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants