Project

General

Profile

Actions

action #81899

closed

coordination #154777: [saga][epic] Shareable os-autoinst and test distribution plugins

coordination #108527: [epic] os-autoinst wheels for scalable code reuse of helper functions and segmented test distributions

Move code from isotovideo to a module size:M

Added by tinita over 3 years ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Low
Assignee:
Category:
Feature requests
Target version:
Start date:
2021-01-08
Due date:
% Done:

0%

Estimated time:

Description

Motivation

Having code in a module (ideally under lib/) and seperated into functions makes it easier to test and mock.

Suggestions

  • Move code out of isotovideo, no files/cmake involved here

Related issues 4 (0 open4 closed)

Related to openQA Project - action #108530: os-autoinst wheels: x11_start_program from os-autoinst-distri-openQA dynamically loaded from another git repo size:MResolvedlivdywan2022-03-17

Actions
Related to openQA Project - action #120786: Jobs are now incomplete when postfail hook fails size:SResolvedmkittler2022-11-21

Actions
Related to openQA Project - action #121045: Performance regression of `os-autoinst` size:MResolvedtinita2022-11-282023-01-20

Actions
Related to openQA Project - action #157339: os-autoinst t/14-isotovideo.t is again taking too long (>20s on my setup) size:MResolvedybonatakis2024-03-15

Actions
Actions #1

Updated by livdywan over 3 years ago

Also, as I found during investigations of unit test execution times in #75232, this would speed up trivial operations like --help or --version which should be fast but take just as much time currently as preparing for a test run.

Actions #2

Updated by okurz about 2 years ago

  • Tags set to os-autoinst, easy, beginner, entrance level
  • Subject changed from Move code from isotovideo a module to [easy][beginner] Move code from isotovideo to a module
  • Description updated (diff)
Actions #3

Updated by okurz about 2 years ago

  • Status changed from New to In Progress
  • Assignee set to okurz
  • Priority changed from Normal to Low
  • Target version changed from future to Ready

We need that for #108530

Actions #6

Updated by okurz about 2 years ago

  • Subject changed from [easy][beginner] Move code from isotovideo to a module to [easy][beginner] Move code from isotovideo to a module size:M
Actions #7

Updated by okurz about 2 years ago

  • Related to action #108530: os-autoinst wheels: x11_start_program from os-autoinst-distri-openQA dynamically loaded from another git repo size:M added
Actions #8

Updated by okurz about 2 years ago

  • Parent task set to #108527
Actions #9

Updated by okurz about 2 years ago

  • Status changed from In Progress to Feedback

Waiting for PR to be merged and switching over to #99663 to not get into my own way.

Actions #10

Updated by okurz about 2 years ago

  • Status changed from Feedback to Blocked
Actions #11

Updated by okurz about 2 years ago

  • Status changed from Blocked to Workable
Actions #12

Updated by livdywan over 1 year ago

  • Assignee deleted (okurz)
Actions #13

Updated by livdywan over 1 year ago

  • Description updated (diff)
Actions #14

Updated by livdywan over 1 year ago

  • Tags changed from os-autoinst, easy, beginner, entrance level to os-autoinst
  • Subject changed from [easy][beginner] Move code from isotovideo to a module size:M to Move code from isotovideo to a module size:M
Actions #15

Updated by livdywan over 1 year ago

  • Status changed from Workable to In Progress

I have an idea on how to move stuff into a backend class, will propose a proof of concept shortly

Actions #16

Updated by okurz over 1 year ago

  • Assignee set to livdywan

@cdywan I assume you forgot to assign yourself when you set to "In Progress" in https://progress.opensuse.org/issues/81899#note-15

Actions #17

Updated by livdywan over 1 year ago

  • Assignee deleted (livdywan)

Here's my proposal to move backend process logic into a new class: https://github.com/os-autoinst/os-autoinst/pull/2194

I was originally thinking to move more into that class but the way global and semi-global variables are used I decided to start smaller.

Another part that might be fine to simply move into Utils is the "debuggers", so I put that in a separate branch: https://github.com/os-autoinst/os-autoinst/pull/2195

Actions #18

Updated by okurz over 1 year ago

  • Status changed from In Progress to Workable
Actions #19

Updated by livdywan over 1 year ago

  • Status changed from Workable to In Progress
  • Assignee set to livdywan

Third part, moving the check_asserted_screen logic into the CommandHandler: https://github.com/os-autoinst/os-autoinst/pull/2196/files

Actions #20

Updated by livdywan over 1 year ago

One more piece, the loop, could be moved out of isotovideo: https://github.com/os-autoinst/os-autoinst/pull/2200

Actions #23

Updated by livdywan over 1 year ago

  • Related to action #120786: Jobs are now incomplete when postfail hook fails size:S added
Actions #24

Updated by livdywan over 1 year ago

Apparently the penultimate change caused a regression. So I proposed an improvement to a unit test to reveal the issue. The other change is in fact fine on its own, and now no longer depends on the broken one.

Actions #25

Updated by livdywan over 1 year ago

cdywan wrote:

Apparently the penultimate change caused a regression. So I proposed an improvement to a unit test to reveal the issue. The other change is in fact fine on its own, and now no longer depends on the broken one.

Alas the unit test won't fail regardless of improvements. From Tina's findings it looks like it's affected by tests run before it. TESTS="t/14-debugging-tools.t t/14-isotovideo.t" makes the unit tests pass in CI and container_run_ci. Commenting out the second spawn_debuggers in the test even makes it fail. This may actually be a race condition.

Actions #26

Updated by tinita over 1 year ago

I was able to get the 14-isotovideo.t pass locally, and also fail with running 14-debugging-tools.t before.
But running 14-isotovideo.t passed very rarely, and running 14-debugging-tools.t before made it pass much more often.
But all this only with tools/container_run_ci. Running the tests manually with prove on my host, 4-debugging-tools.t always fails, even with code coverage enabled.

So it might be a timing issue, and it's not clear how we could reliably make it fail or pass.

But I'm working on code to establish the previous initialization order, and that at least works locally and in CI in all test runs so far.

Actions #28

Updated by livdywan over 1 year ago

tinita wrote:

So it might be a timing issue, and it's not clear how we could reliably make it fail or pass.

Actually just enabling coverage, no other changes, allowed me to reproduce it just running the tests normally, but only twice.

Actions #29

Updated by livdywan over 1 year ago

tinita wrote:

https://github.com/os-autoinst/os-autoinst/pull/2216 merged

Unfortunately this seems to have caused a performance regression #121045#note-8

Actions #30

Updated by okurz over 1 year ago

  • Status changed from In Progress to Blocked

blocked by #121045

Actions #31

Updated by tinita over 1 year ago

  • Status changed from Blocked to Feedback

https://github.com/os-autoinst/os-autoinst/pull/2228 was merged and #121045 resolved, so not blocked anymore

Actions #32

Updated by okurz over 1 year ago

  • Related to action #121045: Performance regression of `os-autoinst` size:M added
Actions #33

Updated by livdywan over 1 year ago

  • Status changed from Feedback to Resolved

As of now idsotovideo has been simplified significantly, primarily handling arguments and keeping state to ensure a known exit code. OpenQA::Isotovideo::Runner is used to start the backend, setup autotest, run the main loop and handle commands meaning all important code paths are implemented in other modules suitable for mocking. Code is also no longer heavily relyant on global variables.

I think we can consider this resolved.

Actions #34

Updated by okurz 2 months ago

  • Related to action #157339: os-autoinst t/14-isotovideo.t is again taking too long (>20s on my setup) size:M added
Actions

Also available in: Atom PDF