Rework extracting duplicated code from generating job settings #2959
Conversation
|
Can you please explain what you changed vs. the last time and what additional tests you have executed? |
Codecov Report
@@ Coverage Diff @@
## master #2959 +/- ##
=======================================
Coverage 91.54% 91.54%
=======================================
Files 202 202
Lines 12707 12703 -4
=======================================
- Hits 11632 11629 -3
+ Misses 1075 1074 -1
Continue to review full report at Codecov.
|
|
In this pr, I move |
t/api/02-iso.t
Outdated
| is_deeply( | ||
| $result, | ||
| { | ||
| BUILD => '176.6', | ||
| BUILD_HA => '176.6', | ||
| BUILD_SDK => '176.6', | ||
| HDD_1 => 'opensuse-13.1-i586-176.6@64bit-minimal_with_sdk176.6_installed.qcow2', | ||
| ISO_MAXSIZE => '4700372992', | ||
| QEMUCPU => 'qemu64', | ||
| VERSION => '13.1', | ||
| BACKEND => 'qemu', | ||
| WORKER_CLASS => 'qemu_i586', | ||
| MACHINE => '64bit', | ||
| DVD => '1', | ||
| FLAVOR => 'DVD', | ||
| TEST_SUITE_NAME => 'test', | ||
| ISO => 'openSUSE-13.1-DVD-i586-Build0091-Media.iso', | ||
| DISTRI => 'opensuse', | ||
| SHUTDOWN_NEEDS_AUTH => '1', | ||
| TEST => 'test', | ||
| ARCH => 'i586' | ||
| }, | ||
| 'specified variables were expanded correctly' | ||
| ); |
There was a problem hiding this comment.
To keep this shorter I recommend you only check for relevant entries, e.g. check MACHINE
There was a problem hiding this comment.
If you do not mind, I prefer to check all the settings. Because I also added some variables such as %BUILD_HA%. In my opinion, it is not only used to check MACHINE, but also other variables that need to be replaced.
There was a problem hiding this comment.
The thing is: It should be clear in this test what settings we really care about. Being too specific is harmful because any next contributor that changes something so that the test fails does not understand easily which parts are relevant. Of course you can check for multiple values but not for all. I recommend to use the json_is test method here.
There was a problem hiding this comment.
I modified the code just checking few import variables. If I understand correctly, we cannot use json_is here, because schedule_iso returns return $t->tx->res.
Amrysliu
force-pushed
the
refactor_generate_job_settings
branch
from
9ff347e
to
e3d84d0
Compare
|
@Martchus @kalikiana Would you mind double-checking this pull request? |
Both in the operation `isos post` and `jobs post`, there are some code used to generate job settings, this pr used to reduce the duplicated code.
When doing `isos post`, the machine name was added to the job settings before expanding specified variables, so the %MACHINE% was not replaced correctly.
Amrysliu
force-pushed
the
refactor_generate_job_settings
branch
from
e3d84d0
to
fbba825
Compare
| next if $key eq 'TEST' || $key eq 'MACHINE'; | ||
| $settings{uc $key} = $args->{$key}; | ||
| } | ||
|
|
||
| # make sure that the DISTRI is lowercase | ||
| $settings{DISTRI} = lc($settings{DISTRI}); |
There was a problem hiding this comment.
FYI, this PR caused regression in many tests that use DISTRI in asset names or job dependency settings. The above line must be executed before generate_settings().
There was a problem hiding this comment.
Example: https://openqa.suse.de/tests/4186731#settings
START_AFTER_TEST was expanded to incorrect value, OpenQA failed to find the parent job and register ASSET_1 from it.
PR os-autoinst#2959 caused a regression issue that the `DISTRI` is not converted into lowercase. Here is the comments and example: https://github.com/os-autoinst/openQA/pull/2959/files#r420928584
Related: https://progress.opensuse.org/issues/63883#note-8