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 UI for pausing on assert screen timeout #1727

Merged

Conversation

Martchus
Copy link
Contributor

@Martchus Martchus commented Jul 19, 2018

@Martchus Martchus force-pushed the pause_on_assert_screen_timeout branch from 75568ac to bafce00 Compare July 20, 2018 14:49
@Martchus Martchus force-pushed the pause_on_assert_screen_timeout branch 2 times, most recently from 2a0ee46 to e3210ed Compare July 31, 2018 14:34
@Martchus
Copy link
Contributor Author

So I now have integration/fullstack test for pausing on assert screen timeout. It works locally, however, I don't expect it to pass on Travis because it requires os-autoinst/os-autoinst#993.

I'll also have to extend the tests to cover opening the needle editor when paused on assert_screen failure.

@Martchus Martchus force-pushed the pause_on_assert_screen_timeout branch 2 times, most recently from 1790aa9 to 5a682bf Compare August 2, 2018 09:45
@Martchus
Copy link
Contributor Author

Martchus commented Aug 2, 2018

The fullstack developer test now also opens the needle editor when paused on assert_screen timeout.

This PR can still be tested on e212 btw. To check whether the tests pass on Travis, os-autoinst/os-autoinst#993 must be merged first.

@Martchus Martchus changed the title WIP: Add UI for pausing on assert screen timeout Add UI for pausing on assert screen timeout Aug 3, 2018
@Martchus Martchus force-pushed the pause_on_assert_screen_timeout branch from d33914c to 0e03d86 Compare August 7, 2018 13:08
@coolo
Copy link
Contributor

coolo commented Aug 8, 2018

it's merged now - please rebase?

@Martchus Martchus force-pushed the pause_on_assert_screen_timeout branch from 0e03d86 to 1200edf Compare August 8, 2018 08:02
@codecov
Copy link

codecov bot commented Aug 8, 2018

Codecov Report

Merging #1727 into master will increase coverage by 1.32%.
The diff coverage is 95.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1727      +/-   ##
==========================================
+ Coverage   88.71%   90.03%   +1.32%     
==========================================
  Files         139      139              
  Lines        9738     9747       +9     
==========================================
+ Hits         8639     8776     +137     
+ Misses       1099      971     -128
Impacted Files Coverage Δ
lib/OpenQA/WebAPI/Controller/LiveViewHandler.pm 97.66% <ø> (ø) ⬆️
lib/OpenQA/WebAPI/Plugin/Helpers.pm 94.38% <100%> (+0.06%) ⬆️
lib/OpenQA/WebAPI/Controller/Running.pm 75.59% <100%> (+2.86%) ⬆️
lib/OpenQA/WebAPI/Controller/Step.pm 84.27% <66.66%> (-0.16%) ⬇️
lib/OpenQA/Worker/Jobs.pm 82.79% <94.11%> (+12.93%) ⬆️
lib/OpenQA/Utils.pm 93.9% <0%> (+0.21%) ⬆️
lib/OpenQA/Schema/Result/Jobs.pm 90.45% <0%> (+0.26%) ⬆️
lib/OpenQA/WebAPI/Controller/API/V1/Job.pm 88.07% <0%> (+0.38%) ⬆️
lib/OpenQA/Worker/Common.pm 80.29% <0%> (+0.72%) ⬆️
lib/OpenQA/Schema/Result/JobModules.pm 87.31% <0%> (+0.74%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be92957...882a886. Read the comment docs.

@Martchus
Copy link
Contributor Author

Martchus commented Aug 8, 2018

Rebased and since os-autoinst/os-autoinst#993 has been merged tests pass.

@Martchus
Copy link
Contributor Author

I think it can be merged as soon as the overall fallout is cleaned. The successive PR is deployed on e212.

@foursixnine
Copy link
Member

@Martchus please rebase :)

Better than suggesting to restart the job because the devel
mode allows to reload needles on resume.
* Upload results in the middle of a test module when paused
  so the needle editor can be used at this point
* Take care that not multiple test modules are marked as
  running at the same time
Adjust wait_for_developer_console_contains_log_message so only
log messages since the last successful match are considered.

This should prevent problems when extending 33-developer_mode.t
to pause/resume multiple times.
So when opening the needle editor from the developer mode on pause
the user at least knows why it is not working/available.
The previous commit contains already tests for the error
handling, so let's add a test for the positive case as well.
Reload page after switching to 2nd tab to block until it has finished
loading.

Note that neither opening the 2nd tab (via window.open()) nor switching
to it seems to block until the page has finished loading.

This should prevent:
Error while executing command: no such element: Unable to locate element:
 {"method":"css selector","selector":"#log"}
Test::More::subtest("connect with 2 clients at the same time (use case:
 developer "..., CODE(0x560552f407f0)) called at t/33-developer_mode.t
 line 392
@Martchus Martchus force-pushed the pause_on_assert_screen_timeout branch from 1200edf to 882a886 Compare August 22, 2018 06:53
@foursixnine
Copy link
Member

I just love how flaky the fullstack test is now :)

@foursixnine
Copy link
Member

foursixnine commented Aug 22, 2018

herrera-wtf

@foursixnine foursixnine merged commit bc546cb into os-autoinst:master Aug 22, 2018
@Martchus Martchus deleted the pause_on_assert_screen_timeout branch August 22, 2018 08:29
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.

None yet

3 participants