action #81899
closedcoordination #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.
0%
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
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.
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)
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
Updated by okurz about 2 years ago
Updated by okurz about 2 years ago
https://github.com/os-autoinst/os-autoinst/pull/2031 for signatures (merged), next
https://github.com/os-autoinst/os-autoinst/pull/2032
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
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
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.
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
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
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
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
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
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
Updated by livdywan over 1 year ago
Updated by livdywan over 1 year ago
The final part https://github.com/os-autoinst/os-autoinst/pull/2209
Updated by livdywan over 1 year ago
- Related to action #120786: Jobs are now incomplete when postfail hook fails size:S added
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.
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.
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.
Updated by tinita over 1 year ago
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.
Updated by livdywan over 1 year ago
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
Updated by okurz over 1 year ago
- Related to action #121045: Performance regression of `os-autoinst` size:M added
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.
Updated by okurz about 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