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

Add loading indicator to the test liveview #6023

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

r-richardson
Copy link
Contributor

This PR removes the empty box which was visible in the webui before liveview content is received by the responsible canvas element. It also adds a loading animation indicating to the user that the liveview is still being loaded.

Related Issue: https://progress.opensuse.org/issues/134840

@r-richardson r-richardson force-pushed the liveview_loading_animation_134840 branch from dc75e93 to 2f09fda Compare October 23, 2024 15:19
assets/javascripts/running.js Outdated Show resolved Hide resolved
@kalikiana
Copy link
Member

[15:22:13] t/full-stack.t ......... 31/? Bailout called.  Further testing stopped:  test 't/full-stack.t' exceeds runtime limit of '1800' seconds
FAILED--Further testing stopped: test 't/full-stack.t' exceeds runtime limit of '1800' seconds

assets/javascripts/running.js Outdated Show resolved Hide resolved
assets/javascripts/running.js Outdated Show resolved Hide resolved
@r-richardson r-richardson force-pushed the liveview_loading_animation_134840 branch 2 times, most recently from 7cc1e96 to c3b75de Compare October 24, 2024 14:11
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.

Project coverage is 98.96%. Comparing base (9a22ad5) to head (8a59ae0).
Report is 23 commits behind head on master.

Files with missing lines Patch % Lines
t/ui/18-tests-details.t 85.71% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6023      +/-   ##
==========================================
- Coverage   98.98%   98.96%   -0.02%     
==========================================
  Files         395      395              
  Lines       39344    39381      +37     
==========================================
+ Hits        38944    38975      +31     
- Misses        400      406       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -91,7 +91,6 @@
// live view
canvas {
max-width: 100%;
border: #ddd 1px solid;
Copy link
Member

Choose a reason for hiding this comment

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

I think the border can give some good indication what's going on if there's a problem so consider to keep that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, though it is then visible during loading
canvas_border
i tried including the loading animation within the canvas, though that hid it.

Copy link
Contributor

Choose a reason for hiding this comment

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

i tried including the loading animation within the canvas, though that hid it.

Maybe it helps to assign a high z-index to the loading animation.

t/ui/18-tests-details.t Outdated Show resolved Hide resolved
t/ui/18-tests-details.t Outdated Show resolved Hide resolved
t/ui/18-tests-details.t Outdated Show resolved Hide resolved
t/ui/18-tests-details.t Outdated Show resolved Hide resolved
t/ui/18-tests-details.t Outdated Show resolved Hide resolved
t/ui/18-tests-details.t Outdated Show resolved Hide resolved
t/ui/18-tests-details.t Outdated Show resolved Hide resolved
t/ui/18-tests-details.t Outdated Show resolved Hide resolved
t/ui/18-tests-details.t Outdated Show resolved Hide resolved
@r-richardson r-richardson force-pushed the liveview_loading_animation_134840 branch 5 times, most recently from d7b26ba to 35f7675 Compare October 24, 2024 16:51
t/ui/18-tests-details.t Outdated Show resolved Hide resolved
t/ui/18-tests-details.t Outdated Show resolved Hide resolved
t/ui/18-tests-details.t Outdated Show resolved Hide resolved
Copy link
Member

@okurz okurz Oct 25, 2024

Choose a reason for hiding this comment

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

I tried out your branch and ran OPENQA_TEST_TIMEOUT_DISABLE=1 count-fail-ratio prove -l t/ui/18-tests-details.t and found a sporadic failure, reproduced only once so far:

## Run 1
t/ui/18-tests-details.t .. ok    
All tests successful.
Files=1, Tests=36, 43 wallclock secs ( 0.09 usr  0.02 sys + 10.22 cusr  0.89 csys = 11.22 CPU)
Result: PASS
## count-fail-ratio: Run: 1. Fails: 0. Fail ratio 0±0%. No fails, computed failure probability < 300.00%
## mean runtime: 42950±0 ms
…
## Run 12
t/ui/18-tests-details.t .. 19/? isElementEnabled: stale element reference: stale element not found in the current frame at /home/okurz/local/os-autoinst/openQA/t/ui/../lib/OpenQA/SeleniumTest.pm:78 at /home/okurz/local/os-autoinst/openQA/t/ui/../lib/OpenQA/SeleniumTest.pm line 81.
	OpenQA::SeleniumTest::__ANON__(Test::Selenium::Chrome=HASH(0x55feb6a44540), "Error while executing command: isElementEnabled: stale elemen"..., HASH(0x55feb7dc5d80)) called at /usr/lib/perl5/vendor_perl/5.26.1/Selenium/Remote/Driver.pm line 356
	Selenium::Remote::Driver::catch {...} ("Error while executing command: isElementEnabled: stale elemen"...) called at /usr/lib/perl5/vendor_perl/5.26.1/Try/Tiny.pm line 123
	Try::Tiny::try(CODE(0x55feb7bb82c0), Try::Tiny::Catch=REF(0x55feb7e41c58)) called at /usr/lib/perl5/vendor_perl/5.26.1/Selenium/Remote/Driver.pm line 361
	Selenium::Remote::Driver::__ANON__(CODE(0x55feb65f7508), Test::Selenium::Chrome=HASH(0x55feb6a44540), HASH(0x55feb7dc5d80)) called at (eval 1582) line 1
	Selenium::Remote::Driver::__ANON__(Test::Selenium::Chrome=HASH(0x55feb6a44540), HASH(0x55feb7dc5d80)) called at (eval 1584) line 2
	Selenium::Remote::Driver::_execute_command(Test::Selenium::Chrome=HASH(0x55feb6a44540), HASH(0x55feb7dc5d80)) called at (eval 1554) line 17
	Selenium::Remote::WebElement::_execute_command(Test::Selenium::Remote::WebElement=HASH(0x55feb79fb418), HASH(0x55feb7dc5d80)) called at /usr/lib/perl5/vendor_perl/5.26.1/Selenium/Remote/WebElement.pm line 180
	Selenium::Remote::WebElement::is_enabled(Test::Selenium::Remote::WebElement=HASH(0x55feb79fb418)) called at t/ui/18-tests-details.t line 84
	main::find_candidate_needles() called at t/ui/18-tests-details.t line 654
	main::test_with_error(1, 0, ARRAY(0x55feb7e46a90), HASH(0x55feb7e3bd08), "100%, 52%") called at t/ui/18-tests-details.t line 674
	main::__ANON__() called at /usr/lib/perl5/vendor_perl/5.26.1/Test/Builder.pm line 374
	eval {...} called at /usr/lib/perl5/vendor_perl/5.26.1/Test/Builder.pm line 374
	Test::Builder::subtest(Test::Builder=HASH(0x55feac77c2a0), "test candidate list", CODE(0x55feb768dda8)) called at /usr/lib/perl5/vendor_perl/5.26.1/Test/More.pm line 831
	Test::More::subtest("test candidate list", CODE(0x55feb768dda8)) called at t/ui/18-tests-details.t line 706
t/ui/18-tests-details.t .. 20/? # Tests were run but no plan was declared and done_testing() was not seen.
t/ui/18-tests-details.t .. Dubious, test returned 254 (wstat 65024, 0xfe00)
All 20 subtests passed 
Test Summary Report
-------------------
t/ui/18-tests-details.t (Wstat: 65024 Tests: 20 Failed: 0)
  Non-zero exit status: 254
  Parse errors: No plan found in TAP output
Files=1, Tests=20, 34 wallclock secs ( 0.07 usr  0.00 sys +  8.60 cusr  1.06 csys =  9.73 CPU)
Result: FAIL
## count-fail-ratio: Run: 12. Fails: 1. Fail ratio 8.33±15.63%
## mean runtime: 39634±2031.91 ms

final results of 20 runs

## count-fail-ratio: Run: 20. Fails: 1. Fail ratio 5.00±9.55%
## mean runtime: 40402±1946.96 ms

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is the same error i've also been running into. Is this maybe related to the line
tests-details.t:311 $driver->execute_script('window.enableStatusUpdates = false'); ?
I tried to move this line directly after my tests, though that did not help.

Copy link
Member

Choose a reason for hiding this comment

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

By the way I compared that to origin/master. Result:

## count-fail-ratio: Run: 30. Fails: 0. Fail ratio 0±0%. No fails, computed failure probability < 10.00%
## mean runtime: 36969±1693.07 ms

so it's very likely error you see is caused by your change.

var scrn = new Image();
scrn.onload = function () {
canvas.width = this.width;
canvas.height = this.height;
context.clearRect(0, 0, this.width, this.width);
context.drawImage(this, 0, 0);

// hide loading animation after the first image is loaded
var loadingElement = document.getElementById('liveview-loading');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var loadingElement = document.getElementById('liveview-loading');
const loadingElement = document.getElementById('liveview-loading');

Comment on lines 333 to 336
var loadingElement = document.getElementById('liveview-loading');
if (loadingElement) {
loadingElement.style.display = 'none';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The element is likely always present at the point this callback is invoked. You could check whether that's the case and if it is the case remove the condition.

@r-richardson r-richardson force-pushed the liveview_loading_animation_134840 branch 7 times, most recently from 8f16459 to 8a59ae0 Compare October 28, 2024 15:14
Copy link
Contributor

mergify bot commented Oct 30, 2024

This pull request is now in conflicts. Could you fix it? 🙏

This PR removes the empty box which was visible in the webui before liveview content is received by the responsible canvas element.
It also adds a loading animation indicating that the liveview is being loaded.
Test cases are also added to confirm the elements are displayed and hidden as expected.

Related Issue: https://progress.opensuse.org/issues/134840
@r-richardson r-richardson force-pushed the liveview_loading_animation_134840 branch from 8a59ae0 to 32b51a2 Compare October 30, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants