action #99663
closedcoordination #80142: [saga][epic] Scale out: Redundant/load-balancing deployments of openQA, easy containers, containers on kubernetes
coordination #99579: [epic][retro] Follow-up to "Published QCOW images appear to be uncompressed"
coordination #99660: [epic] Use more perl signatures in our perl projects
Use more perl signatures - os-autoinst size:M
Description
Updated by okurz over 2 years ago
- Status changed from New to In Progress
Already started with that in https://github.com/os-autoinst/os-autoinst/pull/1773
Updated by okurz over 2 years ago
I had started already here but as I recurringly fail to finish I am splitting this up in multiple PRs for easier testing and planned merges one at a time:
- DONE https://github.com/os-autoinst/os-autoinst/pull/1793 - t/data, merged
- DONE https://github.com/os-autoinst/os-autoinst/pull/1792 - t, merged
- DONE https://github.com/os-autoinst/os-autoinst/pull/1794 - tools, merged
- DONE https://github.com/os-autoinst/os-autoinst/pull/1791 - OpenQA/, merged
- https://github.com/os-autoinst/os-autoinst/pull/1789 - consoles, multiple errors, needs work but should be merged later anyway
- https://github.com/os-autoinst/os-autoinst/pull/1790 - backend, same as above
let's see which ones succeed. Based on criticality we can decide which ones to merge first. I suggest to first do low-risk ones like tools and t/, later backends and consoles
EDIT: Updated current status of PRs
Updated by okurz over 2 years ago
- Copied to action #100967: Use more perl signatures - openQA size:M added
Updated by okurz over 2 years ago
additional PR for myjsonrpc only: https://github.com/os-autoinst/os-autoinst/pull/1827 - merged
Updated by okurz over 2 years ago
- Related to action #102146: Deprecate os-autoinst backend::pvm added
Updated by okurz over 2 years ago
I now merged https://github.com/os-autoinst/os-autoinst/pull/1790 "use signatures in os-autoinst backend" as it was ready and has approval. I will have alert duty this week so hopefully I would see any fallout :)
Updated by okurz over 2 years ago
There was no feedback on the last merged pull request so I assume we are good and can continue with the next, i.e. "consoles"
Updated by livdywan over 2 years ago
okurz wrote:
There was no feedback on the last merged pull request so I assume we are good and can continue with the next, i.e. "consoles"
Updated by okurz over 2 years ago
I am playing it safe and doing the first part of consoles with https://github.com/os-autoinst/os-autoinst/pull/1868 (merged). https://github.com/os-autoinst/os-autoinst/pull/1789 includes the second part. I realized that a good additional test is to execute the openQA full stack test while I have my signatures changes in os-autoinst checked out locally. Seems like maybe the os-autoinst tests do not cover the complete interface for the interactive mode and such so I will test my changes with openQA first.
Updated by okurz over 2 years ago
- Status changed from Feedback to In Progress
prepared https://github.com/os-autoinst/os-autoinst/pull/1899 for parts of the consoles and merged. Continuing with consoles in https://github.com/os-autoinst/os-autoinst/pull/1789
Updated by okurz over 2 years ago
- Related to action #104986: tests incomplete with: auto_review:"backend died: Too many arguments for subroutine.*consoles::vnc_base::get_last_mouse_set":retry added
Updated by okurz over 2 years ago
- Related to action #104520: Move svirt extract_asset code from os-autoinst-distri-opensuse to os-autoinst/backend/svirt.pm size:M auto_review:"unable to extract assets: Can't call method.+name.+on an undefined value":retry added
Updated by okurz over 2 years ago
- Status changed from In Progress to Feedback
The PR https://github.com/os-autoinst/os-autoinst/pull/1789 is basically ready but so far nobody dared to merge it due to the lower statement code coverage of consoles::VNC. One simple related change which actually also touches consoles::VNC is https://github.com/os-autoinst/os-autoinst/pull/1934 about the "ikvm" backend. Currently https://app.codecov.io/gh/os-autoinst/os-autoinst/blob/master/consoles/VNC.pm reports 57%. I rebased https://github.com/os-autoinst/os-autoinst/pull/1789 and will see what this will end up with as coverage. Previously the patch coverage in the PR was reported as 67%.
Updated by okurz about 2 years ago
surprisngly cdywan merged https://github.com/os-autoinst/os-autoinst/pull/1789 . I assume they missed our discussions and plans. Regardless, I checked jobs on https://openqa.opensuse.org/tests and found no related failures so I guess we are good with that change. I can now continue with https://github.com/os-autoinst/os-autoinst/pull/1773 but that's still a rather big PR so maybe split out more files.
Updated by livdywan about 2 years ago
okurz wrote:
surprisngly cdywan merged https://github.com/os-autoinst/os-autoinst/pull/1789 . I assume they missed our discussions and plans. Regardless, I checked jobs on https://openqa.opensuse.org/tests and found no related failures so I guess we are good with that change.
2 approvals, 2 other reviewers, no objections after having brought up concerns with coverage. I sanity-checked also. Typically we merge in such cases. If you clearly didn't want it merged I suggest a blocking review, draft or not-ready label for the future.
I can now continue with https://github.com/os-autoinst/os-autoinst/pull/1773 but that's still a rather big PR so maybe split out more files.
How about consoles+backends and api(mmapi/bmwqemu/distribution) or something along those lines?
Updated by okurz about 2 years ago
cdywan wrote:
okurz wrote:
surprisngly cdywan merged https://github.com/os-autoinst/os-autoinst/pull/1789 . I assume they missed our discussions and plans. Regardless, I checked jobs on https://openqa.opensuse.org/tests and found no related failures so I guess we are good with that change.
2 approvals, 2 other reviewers, no objections after having brought up concerns with coverage. I sanity-checked also. Typically we merge in such cases. If you clearly didn't want it merged I suggest a blocking review, draft or
not-readylabel for the future.
sure, not your fault at all but mine and it seems all turned out well.
I can now continue with https://github.com/os-autoinst/os-autoinst/pull/1773 but that's still a rather big PR so maybe split out more files.
How about consoles+backends and api(mmapi/bmwqemu/distribution) or something along those lines?
yes, that's what I am thinking.
Updated by okurz about 2 years ago
- Related to action #106654: [ipmi][openqa][vnc] Massive test run failures with 'IO::Socket::INET: connect: Connection refused' due to "Use of uninitialized value.*connect_timeout in addition.*consoles/VNC.pm line 13.*":retry added
Updated by okurz about 2 years ago
- Related to action #108323: Subroutine consoles::sshVirtsh::has redefined at .../Class/Accessor.pm added
Updated by okurz about 2 years ago
one more step: https://github.com/os-autoinst/os-autoinst/pull/2006 merged
Updated by okurz almost 2 years ago
- Subject changed from Use more perl signatures - os-autoinst to Use more perl signatures - os-autoinst size:M
Updated by okurz almost 2 years ago
https://github.com/os-autoinst/os-autoinst/pull/2046 for commands.pm . Should wait a day or two for feedback before continuing.
Updated by okurz almost 2 years ago
- Status changed from Feedback to In Progress
PR merged, no problems observed. Continuing with https://github.com/os-autoinst/os-autoinst/pull/1773 , now merged as well. Continuing with more files which have received non-signature changes since then and some places which I might have missed and testapi.pm itself as well.
Continuing in https://mysuse.sharepoint.com/:v:/s/HowWeWork/ERL9nT8YLmNOnfjj5G53MfUBE2rFh1BeYA7kNTxhZpC4UQ?e=iYvr7b
Updated by okurz almost 2 years ago
- Related to action #110983: Wrong signatures auto_review:"Too.*arguments for subroutine":retry added
Updated by okurz almost 2 years ago
Brought up in weekly 2022-05-13. I was making a stupid mistake on trying to just add -signatures in one commit and see how that goes but of course that stumbles already over the prototypes used. So I should definitely use both together.
I am welcoming everybody to review https://github.com/os-autoinst/os-autoinst/pull/1696 and help me with some suggestions.
Updated by okurz almost 2 years ago
https://github.com/os-autoinst/os-autoinst/pull/2060 merged including parts of the above including a syntax check to ensure we have signatures in all perl files, except testapi.pm so far.
Updated by okurz almost 2 years ago
- Status changed from In Progress to Feedback
Updated by okurz almost 2 years ago
As this PR still turns out to be too big to be easily review I am splitting out yet another PR:
Updated by okurz almost 2 years ago
To prevent breakage in os-autoinst-distri-opensuse I prepared three PRs:
Updated by okurz almost 2 years ago
All three merged. Now I created https://github.com/os-autoinst/os-autoinst/pull/2078 which should include only the testapi.pm changes that are covered by automatic tests.
Updated by okurz almost 2 years ago
- Related to action #112319: Better and earlier checks of test code against "wrong API usage" added
Updated by okurz almost 2 years ago
- Status changed from Feedback to Workable
https://github.com/os-autoinst/os-autoinst/pull/1696 . Haven't found anymore problems on openqaworker7. I suggest we continue with some unit tests in a mob programming session.
Updated by okurz almost 2 years ago
- Related to coordination #109740: [epic] Stable os-autoinst unit tests with good coverage added
Updated by livdywan almost 2 years ago
okurz wrote:
https://github.com/os-autoinst/os-autoinst/pull/1696 . Haven't found anymore problems on openqaworker7. I suggest we continue with some unit tests in a mob programming session.
testapi unit tests, as opposed to #94952 which covers basetest
Updated by okurz over 1 year ago
Updated by okurz about 1 year ago
- Status changed from Workable to Feedback
- Target version changed from future to Ready
https://github.com/os-autoinst/os-autoinst/pull/1696 rebased on top of https://github.com/os-autoinst/os-autoinst/pull/2252 and https://github.com/os-autoinst/os-autoinst/pull/2256 with all CI checks passed, i.e. the changed lines are fully covered in tests (not the complete functions but only the signature relevant lines). So finally the complete PR should be ready including enforcement of using signatures in all perl files in this project.
Updated by okurz about 1 year ago
- Due date set to 2023-03-03
https://github.com/os-autoinst/os-autoinst/pull/1696 and https://github.com/os-autoinst/os-autoinst/pull/2256 merged as well. If there are no corresponding regressions within the next days we can resolve.
Updated by okurz about 1 year ago
- Related to action #124538: generation of png from sound files creates a backtrace auto_review:"sh:.*snd2png.*HASH":retry added
Updated by livdywan about 1 year ago
Updated by okurz about 1 year ago
- Due date deleted (
2023-03-03) - Status changed from Feedback to Resolved
merged and deployed, https://openqa.opensuse.org/tests/3120074#step/firefox_audio/9 fixed. With this I consider this ticket done.