PART 2: author tests + common perlcritic - After #7068 by okurz · Pull Request #7162 · os-autoinst/openQA

okurz · 2026-03-20T21:48:51Z

After:

test: Move author tests to xt/ and rename target to test-author #7068

subrepo: subdir: "external/os-autoinst-common" merged: "8fffc1f59" upstream: origin: "git@github.com:os-autoinst/os-autoinst-common.git" branch: "master" commit: "8fffc1f59" git-subrepo: version: "0.4.6" origin: "???" commit: "???"

Motivation: Following Perl ecosystem best practices, author tests (static analysis, style checks, POD, etc.) should be separated from functional/dynamic tests. This allows build environments and package maintainers to focus exclusively on tests that verify the correctness of the software in its runtime environment, while avoiding dependencies and runtime overhead associated with developer-only quality gates. Furthermore, compiling the entire codebase via `01-compile-check-all.t` takes exceptionally long when running alongside other tests because it gets instrumented by `Devel::Cover`. Separating it into its own directory allows us to isolate it and bypass the coverage overhead. Design Choices: - Moved 00-tidy.t, 01-style.t, 02-pod.t, and 45-make-update-deps.t from t/ to xt/. - Moved 01-compile-check-all.t from t/ to t/compile/. - Renamed the Makefile target 'test-tidy-compile-style' to 'test-author' to match industry-standard naming and ensure consistency with the 'os-autoinst-common' sub-module. - Added a new 'test-compile' Makefile target for the compile checks. - Set COVEROPT="" on both 'test-author' and 'test-compile' targets to bypass Devel::Cover instrumentation, drastically speeding up execution. - Updated t/01-compile-check-all.t to include the new xt/ directory in its syntax checks. - Updated xt/01-style.t to include xt/ in style checks and correctly exclude itself from self-referential style rules. - Simplified Makefile targets (test-t, test-ui, test-api) by removing GLOBIGNORE exclusions for moved tests, as they are naturally isolated. - Updated t/testrules.yml to reflect the new test locations. - Simplified dist/rpm/openQA.spec as author tests can be ignored completely. - Updated `prove_wrapper` in CI tools to support new directories. User Benefit: - Developers get much faster feedback via 'make test-author' and 'make test-compile' due to bypassing Devel::Cover. - CI and build environments can more easily exclude non-functional tests, speeding up build-time verification and reducing dependency bloat. - Improved project structure consistency with other projects in the os-autoinst ecosystem.

Motivation: Compiling the entire codebase (`01-compile-check-all.t`) is functionally a dynamic test that requires all application dependencies to be fully installed. It logically does not belong with static `checkstyle` tools (like yamllint, perltidy) which do not execute application code. Furthermore, isolating it into its own CI job allows it to run concurrently alongside both static analysis and standard unit testing, lowering total pipeline execution time. Design Choices: - Moved `test-compile` out of the `test-checkstyle` Makefile target. - Added a dedicated `compile` job to `.circleci/config.yml` workflow. User Benefits: - Developers running `make test-checkstyle` get instant feedback on linting and style errors without waiting for a full module compile. - Improved CI execution parallelism.

Motivation: Preparation for using os-autoinst-common perlcritic Related issue: https://progress.opensuse.org/issues/188058

Motivation: Preparation for using os-autoinst-common perlcritic The alternative would be `no critic (RedundantStrictWarning)` on every call of `use Mojo::Base -signatures;` Related issue: https://progress.opensuse.org/issues/188058

Motivation: Preparation for using os-autoinst-common perlcritic Related issue: https://progress.opensuse.org/issues/188058

github-actions · 2026-03-20T21:49:01Z

Great PR! Please pay attention to the following items before merging:

Files matching external/**:

Consider doing this change in the upstream repository directly, see external/os-autoinst-common/README.md

This is an automatically generated QA checklist based on modified files.

perlpunk · 2026-03-21T00:20:51Z

Makefile

+
+.PHONY: test-compile
+test-compile: ## Run compile tests
+	$(MAKE) test-unit-and-integration TIMEOUT_M=20 PROVE_ARGS="$$HARNESS t/compile/*.t" GLOBIGNORE="$(unstables)" COVEROPT=""


I think I commented elsewhere in another PR with the same change that it could theoretically happen that a file (script or module) goes completely uncovered if it isn't tested at all, and the compile check doesn't run with coverage.
Just saying, in my role as the chief scepticism officer ;-)

perlpunk · 2026-03-21T00:26:23Z

script/openqa-load-templates

-                    $res = post_yaml_templates($jg->{name}, "scenarios: {}\nproducts: {}\n");
-                    last unless $res->is_success;
-                }
+        unless ($res->is_success) {


I don't like too much refactoring in a commit titled fix: resolve perlcritic violations in scripts. Was perlcritic complaining about the complexity here?

perlpunk · 2026-03-21T00:36:59Z

I checked out your PR and tried to run one test:

% make test TESTS=t/api/04-jobs.t
make test-unit-and-integration TIMEOUT_M=20 PROVE_ARGS="$HARNESS t/compile/*.t" GLOBIGNORE="" COVEROPT=""
make[1]: Entering directory '/home/tina/openqadev.leap/repos/openQA'
export GLOBIGNORE="";\
export DEVEL_COVER_DB_FORMAT=JSON;\
export PERL5OPT=" -It/lib -I/home/tina/openqadev.leap/repos/openQA/t/lib -I/home/tina/openqadev.leap/repos/openQA/external/os-autoinst-common/lib  -MOpenQA::Test::PatchDeparse";\
RETRY=0 HOOK=./tools/delete-coverdb-folder timeout --foreground -s SIGINT -k 5 -v $((20 * 1 * (0 + 1) ))m tools/retry "prove" -l t/compile/*.t
t/compile/01-compile-check-all.t .. 742/? ^Ctimeout: sending signal INT to command ‘tools/retry’
make[1]: *** [Makefile:276: test-unit-and-integration] Interrupt
make: *** [Makefile:374: test-compile] Interrupt

I find it a bit unfortunate that the simple make test target doesn't use TESTS any more. So I have to use test-with-database now, if I want to run only one test.

perlpunk · 2026-03-21T01:04:35Z

I fixed some tests and didn't want to create a single suggestion for each:

diff

diff --git a/t/05-scheduler-dependencies.t b/t/05-scheduler-dependencies.t
index 3cbab68b9..4eb54d64f 100644
--- a/t/05-scheduler-dependencies.t
+++ b/t/05-scheduler-dependencies.t
@@ -715,7 +715,7 @@ subtest 'duplicate parallel parent in tree with all dependency types' => sub {
 
     # check chained parents
     @sorted_got = sort @{$jobTA2->{parents}->{Chained}};
-    @sorted_exp = sort;
+    @sorted_exp = ();
     is_deeply \@sorted_got, \@sorted_exp, 'jobTA2 not regularly chained after jobQ' or always_explain \@sorted_got;
 
     # check directly chained parents
diff --git a/t/05-scheduler-serialize-directly-chained-dependencies.t b/t/05-scheduler-serialize-directly-chained-dependencies.t
index 52485fabd..63cb88f80 100644
--- a/t/05-scheduler-serialize-directly-chained-dependencies.t
+++ b/t/05-scheduler-serialize-directly-chained-dependencies.t
@@ -141,7 +141,7 @@ $cluster_info{$_}->{state} = SCHEDULED for (1, 9);
 # provide a sort function to control the order between multiple children of the same parent
 my %sort_criteria = (12 => 'anfang', 7 => 'danach', 6 => 'mitte', 8 => 'nach mitte', 1 => 'zuletzt');
 my $sort_function = sub {
-    return [sort { ($sort_criteria{$a} // $a) cmp($sort_criteria{$b} // $b) } @{shift}];
+    return [sort { ($sort_criteria{$a} // $a) cmp($sort_criteria{$b} // $b) } @{+shift}];
 };
 @expected_sequence = (0, 12, 7, 6, [8, [10, 11], 9], [1, [2, 3], [4, 5]]);
 ($computed_sequence, $visited)
diff --git a/t/30-test_parser.t b/t/30-test_parser.t
index e9bdbf0c1..fff41762d 100644
--- a/t/30-test_parser.t
+++ b/t/30-test_parser.t
@@ -84,7 +84,7 @@ package Dummy2to {    # uncoverable statement count:2
         @{$self} = qw(a b c);
         return $self;
     }
-    sub to_array { [@{shift}] }
+    sub to_array { [@{+shift}] }
 }    # uncoverable statement
 
 package Dummy3to {    # uncoverable statement count:2
@@ -96,7 +96,7 @@ package Dummy3to {    # uncoverable statement count:2
         $self->{foobar} = 'barbaz';
         return $self;
     }
-    sub to_hash { return {%{shift}} }
+    sub to_hash { return {%{+shift}} }
 }    # uncoverable statement
 
 package Dummy3 {    # uncoverable statement count:2
@@ -832,7 +832,7 @@ subtest 'Unstructured data' => sub {
     # However it will require a specific parser implementation to handle the data.
     # Then we can map results as general OpenQA::Parser::Result objects.
     my $parser = OpenQA::Parser::UnstructuredDummy->new();
-    $parser->parse join '', <DATA>;
+    $parser->parse(join '', <DATA>);
     ok $parser->results->size == 5, 'There are some results';
 
     $parser->results->each(
diff --git a/t/31-api_descriptions.t b/t/31-api_descriptions.t
index 7dadb1cfb..2d0ebd60f 100644
--- a/t/31-api_descriptions.t
+++ b/t/31-api_descriptions.t
@@ -9,6 +9,7 @@ use Test::Output qw(combined_like);
 
 use File::Temp qw(tempdir);
 use Mojo::File qw(path);
+use Mojolicious;
 
 use experimental 'signatures';
 
diff --git a/t/43-cli-api.t b/t/43-cli-api.t
index 67de307de..1f69ab414 100644
--- a/t/43-cli-api.t
+++ b/t/43-cli-api.t
@@ -4,6 +4,7 @@
 use Test::Most;
 use Test::Warnings qw(:all :report_warnings);
 use experimental 'signatures';
+use utf8;
 
 use FindBin;
 use lib "$FindBin::Bin/lib", "$FindBin::Bin/../external/os-autoinst-common/lib";
diff --git a/t/api/04-jobs.t b/t/api/04-jobs.t
index 7f208a4aa..b4442e47e 100644
--- a/t/api/04-jobs.t
+++ b/t/api/04-jobs.t
@@ -568,7 +568,7 @@ subtest 'Test failure - if chunks are broken' => sub {
     $pieces->each(
         sub {
             $_->prepare;
-            $_->content int rand 99999;
+            $_->content(int rand 99999);
             my $chunk_asset = Mojo::Asset::Memory->new->add_chunk($_->serialize);
             $t->post_ok('/api/v1/jobs/99963/artefact' => form =>
                   {file => {file => $chunk_asset, filename => 'hdd_image.qcow2'}, asset => 'public'});
@@ -588,7 +588,7 @@ subtest 'last chunk is broken' => sub {
     # That will fail during total cksum calculation
     $pieces->each(
         sub {
-            $_->content int rand 99999 if $_->is_last;
+            $_->content(int rand 99999) if $_->is_last;
             $_->prepare;
             my $chunk_asset = Mojo::Asset::Memory->new->add_chunk($_->serialize);
             $t->post_ok('/api/v1/jobs/99963/artefact' => form =>
diff --git a/t/ui/23-audit-log.t b/t/ui/23-audit-log.t
index d8d223dbf..2a4e66081 100644
--- a/t/ui/23-audit-log.t
+++ b/t/ui/23-audit-log.t
@@ -4,6 +4,7 @@
 
 use Test::Most;
 use experimental 'signatures';
+use feature qw(state);
 
 use FindBin;
 use lib "$FindBin::Bin/../lib", "$FindBin::Bin/../../external/os-autoinst-common/lib";

perlpunk · 2026-03-21T10:09:08Z

I think I can fix the HashKeyQuotes and the RedundantStrictWarnings policy, so you don't need to work around them

perlpunk · 2026-03-21T10:39:07Z

t/24-worker-jobs.t

    my $test_image = 'test.png';
    path($job->_result_file_path(".thumbs/$test_image"))->touch;
-    $job->images_to_send->{'098f6bcd4621d373cade4e832627b4f6'} = $test_image;
+    $job->images_to_send->{'098f6bcd4621d373cade4e832627b4f6'} = $test_image;    ## no critic (OpenQA::HashKeyQuotes)


The no critic should not be needed after os-autoinst/os-autoinst-common#74

perlpunk · 2026-03-21T11:39:32Z

t/api/02-iso-download.t


 use Test::Most;
-use Mojo::Base -base, -signatures;
+use experimental 'signatures';


This won't work in cases where Mojo::Base is actually used as a base class.
However it shouldn't be needed anymore after os-autoinst/os-autoinst-common#75

okurz · 2026-03-21T11:56:00Z

I checked out your PR and tried to run one test:

% make test TESTS=t/api/04-jobs.t
make test-unit-and-integration TIMEOUT_M=20 PROVE_ARGS="$HARNESS t/compile/*.t" GLOBIGNORE="" COVEROPT=""
make[1]: Entering directory '/home/tina/openqadev.leap/repos/openQA'
export GLOBIGNORE="";\
export DEVEL_COVER_DB_FORMAT=JSON;\
export PERL5OPT=" -It/lib -I/home/tina/openqadev.leap/repos/openQA/t/lib -I/home/tina/openqadev.leap/repos/openQA/external/os-autoinst-common/lib  -MOpenQA::Test::PatchDeparse";\
RETRY=0 HOOK=./tools/delete-coverdb-folder timeout --foreground -s SIGINT -k 5 -v $((20 * 1 * (0 + 1) ))m tools/retry "prove" -l t/compile/*.t
t/compile/01-compile-check-all.t .. 742/? ^Ctimeout: sending signal INT to command ‘tools/retry’
make[1]: *** [Makefile:276: test-unit-and-integration] Interrupt
make: *** [Makefile:374: test-compile] Interrupt

I find it a bit unfortunate that the simple make test target doesn't use TESTS any more. So I have to use test-with-database now, if I want to run only one test.

Hm, it does use TESTS but as test: is now calling test-compile test-with-database it will call the test-compile unconditionally without evaluating TESTS. To be handled in #7068

okurz added 13 commits March 20, 2026 22:47

feat(Makefile): add help text for missing targets

1033f37

fix: resolve perlcritic violations in scripts

5169bd6

Motivation: Preparation for using os-autoinst-common perlcritic Related issue: https://progress.opensuse.org/issues/188058

fix: resolve perlcritic violations in t/ui/

029e499

Motivation: Preparation for using os-autoinst-common perlcritic Related issue: https://progress.opensuse.org/issues/188058

fix: resolve perlcritic violations in tools

c4eaa77

Motivation: Preparation for using os-autoinst-common perlcritic Related issue: https://progress.opensuse.org/issues/188058

fix: resolve perlcritic violations in xt/

20daf06

Motivation: Preparation for using os-autoinst-common perlcritic Related issue: https://progress.opensuse.org/issues/188058

fix: resolve perlcritic violations in t/api/

0fe1e2b

Motivation: Preparation for using os-autoinst-common perlcritic Related issue: https://progress.opensuse.org/issues/188058

fix: resolve perlcritic violations in t/

960f134

Motivation: Preparation for using os-autoinst-common perlcritic Related issue: https://progress.opensuse.org/issues/188058

fix: resolve perlcritic violations in t/lib/

5535045

Motivation: Preparation for using os-autoinst-common perlcritic Related issue: https://progress.opensuse.org/issues/188058

chore: use os-autoinst-common perlcritic

8178437

perlpunk reviewed Mar 21, 2026

View reviewed changes

okurz marked this pull request as draft March 21, 2026 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PART 2: author tests + common perlcritic - After #7068#7162

PART 2: author tests + common perlcritic - After #7068#7162
okurz wants to merge 13 commits intoos-autoinst:masterfrom
okurz:feature/036_move_author_tests_to_xt_and_perlcritic

okurz commented Mar 20, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Mar 20, 2026 •

edited by okurz

Loading

Uh oh!

perlpunk Mar 21, 2026

Uh oh!

perlpunk Mar 21, 2026

Uh oh!

perlpunk commented Mar 21, 2026

Uh oh!

perlpunk commented Mar 21, 2026

Uh oh!

perlpunk commented Mar 21, 2026

Uh oh!

perlpunk Mar 21, 2026

Uh oh!

perlpunk Mar 21, 2026

Uh oh!

okurz commented Mar 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

okurz commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 20, 2026 • edited by okurz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

perlpunk Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

perlpunk Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

perlpunk commented Mar 21, 2026

Uh oh!

perlpunk commented Mar 21, 2026

Uh oh!

perlpunk commented Mar 21, 2026

Uh oh!

perlpunk Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

perlpunk Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

okurz commented Mar 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

okurz commented Mar 20, 2026 •

edited

Loading

github-actions bot commented Mar 20, 2026 •

edited by okurz

Loading