Project

General

Profile

Actions

action #99663

closed

coordination #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

Added by okurz over 2 years ago. Updated over 1 year ago.

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

0%

Estimated time:

Description

Motivation

See #99660. Use signatures to prevent unintended misuses of function signatures, here for os-autoinst


Related issues 10 (3 open7 closed)

Related to openQA Project - action #102146: Deprecate os-autoinst backend::pvmResolvedokurz2021-11-092021-11-26

Actions
Related to openQA Project - action #104986: tests incomplete with: auto_review:"backend died: Too many arguments for subroutine.*consoles::vnc_base::get_last_mouse_set":retryResolvedokurz2022-01-18

Actions
Related to openQA Project - 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":retryWorkable2021-12-29

Actions
Related to openQA Project - 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.*":retryResolvedokurz2022-02-11

Actions
Related to openQA Project - action #108323: Subroutine consoles::sshVirtsh::has redefined at .../Class/Accessor.pmResolvedmkittler2022-03-14

Actions
Related to openQA Project - action #110983: Wrong signatures auto_review:"Too.*arguments for subroutine":retryResolvedokurz2022-05-12

Actions
Related to openQA Project - action #112319: Better and earlier checks of test code against "wrong API usage"New2022-06-13

Actions
Related to openQA Project - coordination #109740: [epic] Stable os-autoinst unit tests with good coverageResolvedokurz2021-06-30

Actions
Related to openQA Tests - action #124538: generation of png from sound files creates a backtrace auto_review:"sh:.*snd2png.*HASH":retryResolvedokurz2023-02-15

Actions
Copied to openQA Project - action #100967: Use more perl signatures - openQA size:MWorkable

Actions
Actions #1

Updated by okurz over 2 years ago

  • Status changed from New to In Progress
Actions #2

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:

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

Actions #3

Updated by okurz over 2 years ago

3 done, 3 to go

Actions #4

Updated by okurz over 2 years ago

  • Copied to action #100967: Use more perl signatures - openQA size:M added
Actions #5

Updated by okurz over 2 years ago

additional PR for myjsonrpc only: https://github.com/os-autoinst/os-autoinst/pull/1827 - merged

Actions #6

Updated by okurz over 2 years ago

Actions #7

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 :)

Actions #8

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"

Actions #9

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"

https://github.com/os-autoinst/os-autoinst/pull/1868

Actions #10

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.

Actions #11

Updated by okurz over 2 years ago

  • Status changed from In Progress to Feedback

I will wait for some more critical os-autoinst related changes before continuing, e.g. waiting for #103422 and #103611

Actions #12

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

Actions #13

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
Actions #14

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
Actions #15

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%.

Actions #16

Updated by okurz over 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.

Actions #17

Updated by livdywan over 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?

Actions #18

Updated by okurz over 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-ready label 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.

Actions #19

Updated by okurz over 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
Actions #20

Updated by okurz over 2 years ago

  • Related to action #108323: Subroutine consoles::sshVirtsh::has redefined at .../Class/Accessor.pm added
Actions #22

Updated by okurz about 2 years ago

  • Subject changed from Use more perl signatures - os-autoinst to Use more perl signatures - os-autoinst size:M
Actions #23

Updated by okurz about 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.

Actions #24

Updated by okurz about 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

Actions #25

Updated by okurz about 2 years ago

  • Related to action #110983: Wrong signatures auto_review:"Too.*arguments for subroutine":retry added
Actions #26

Updated by okurz about 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.

Actions #27

Updated by okurz about 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.

Actions #28

Updated by okurz about 2 years ago

  • Status changed from In Progress to Feedback
Actions #29

Updated by okurz about 2 years ago

As this PR still turns out to be too big to be easily review I am splitting out yet another PR:

Actions #31

Updated by okurz about 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.

Actions #32

Updated by okurz about 2 years ago

  • Related to action #112319: Better and earlier checks of test code against "wrong API usage" added
Actions #33

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.

Actions #34

Updated by okurz almost 2 years ago

Actions #35

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

Actions #36

Updated by okurz over 1 year ago

  • Target version changed from Ready to future
Actions #38

Updated by okurz over 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.

Actions #39

Updated by okurz over 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.

Actions #40

Updated by okurz over 1 year ago

  • Related to action #124538: generation of png from sound files creates a backtrace auto_review:"sh:.*snd2png.*HASH":retry added
Actions #42

Updated by okurz over 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.

Actions

Also available in: Atom PDF