Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Mojolicious::Routes::Route::any instead of deprecated …::route #1613

Merged
merged 2 commits into from
Jan 18, 2021

Conversation

Martchus
Copy link
Contributor

Otherwise

Mojolicious::Routes::Route::route is DEPRECATED in favor of Mojolicious::Routes::Route::any at …/commands.pm line 253.

shows up in the log.

@Martchus
Copy link
Contributor Author

By the way, I've tested this manually and features depending on that routing still work.

@Martchus
Copy link
Contributor Author

Not sure why the tests fail:

6: #   Failed test 'No tests run for subtest "load test success when casedir and productdir are relative path"'
6: #   at ./14-isotovideo.t line 200.
6: "/usr/bin/perl" failed to start: "No child processes" at ./14-isotovideo.t line 34.

The failure seems unrelated and I can not reproduce it locally. Unfortunately the log archive doesn't contain more verbose output.

@Martchus
Copy link
Contributor Author

The CI error is persistent (tried to restart 2 times). I can not reproduce it locally. I've created an empty PR (#1614) to check whether it also happens on master.

@okurz
Copy link
Member

okurz commented Jan 18, 2021

#1614 only failed in tidy but not in the same test as here where we have:

6:     # No tests run!
6: 
6: #   Failed test 'No tests run for subtest "load test success when casedir and productdir are relative path"'
6: #   at ./14-isotovideo.t line 200.
6: "/usr/bin/perl" failed to start: "No child processes" at ./14-isotovideo.t line 34.
6: # Tests were run but no plan was declared and done_testing() was not seen.
6: # Looks like your test exited with 255 just after 10.
6: [09:58:13] ./14-isotovideo.t ........................ 
6: Dubious, test returned 255 (wstat 65280, 0xff00)
6: Failed 1/10 subtests 

@Martchus
Copy link
Contributor Author

Seems like it is just a matter of luck. After pushing the tidy changes (which should not affect anything) I get the same error on #1614:

 6: Test Summary Report
6: -------------------
6: ./14-isotovideo.t                      (Wstat: 65280 Tests: 10 Failed: 1)
6:   Failed test:  10
6:   Non-zero exit status: 255
6:   Parse errors: No plan found in TAP output
6: Files=36, Tests=924, 583 wallclock secs ( 0.74 usr  0.08 sys + 429.06 cusr 26.54 csys = 456.42 CPU)
6: Result: FAIL
6/6 Test #6: test-perl-testsuite ..............***Failed  583.28 sec

Otherwise
```
Mojolicious::Routes::Route::route is DEPRECATED in favor of Mojolicious::Routes::Route::any at …/commands.pm line 253.
```
shows up in the log.
With `capture` from IPC::System::Simple this test fails very often. This
is reproducible within the CI environment and on my local Tumbleweed
system. When using Perl's built-in `qx()` the same test works fine so it
makes sense to simply use that.
@Martchus
Copy link
Contributor Author

Martchus commented Jan 18, 2021

It seems I was just very lucky when running the test locally. It actually fails there as well most of the time. I hope my fix makes the CI pass as well. See the commit message for more details.

Seems like we'll have to find out whether it also works within the CI because it currently doesn't come very far:

 Using default tag: latest
Error response from daemon: Get https://registry.opensuse.org/v2/: net/http: request canceled (Client.Timeout exceeded while awaiting headers)
Error: Process completed with exit code 1.

note("Starting isotovideo with: @cmd");
capture [$args{exit_code}], @cmd;
note "Starting isotovideo with: @cmd";
my $output = qx(@cmd);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea why qx would be more reliable than capture? I couldn't even repro this with your branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a bug in IPC::Run::Simple which provides capture. However, I'm not really motivated to investigate bugs in IPC::Run::Simple, especially if using built-in functions does not even require much more code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And about the reproducibility: I couldn't reproduce it locally as well until today where it actually almost always failed (untils switching to qx). So I'm not sure what "variable" influences whether the bug is triggered or not.

@codecov
Copy link

codecov bot commented Jan 18, 2021

Codecov Report

Merging #1613 (fe510ff) into master (b18362c) will increase coverage by 0.03%.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1613      +/-   ##
==========================================
+ Coverage   62.18%   62.22%   +0.03%     
==========================================
  Files          58       58              
  Lines        6604     6605       +1     
==========================================
+ Hits         4107     4110       +3     
+ Misses       2497     2495       -2     
Impacted Files Coverage Δ
backend/baseclass.pm 46.39% <ø> (ø)
consoles/VNC.pm 5.24% <0.00%> (ø)
backend/qemu.pm 49.67% <100.00%> (ø)
commands.pm 86.52% <100.00%> (ø)
consoles/sshSerial.pm 75.86% <100.00%> (ø)
isotovideo 89.40% <100.00%> (+0.97%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0e24ed...fe510ff. Read the comment docs.

@mergify mergify bot merged commit 06a8eb3 into os-autoinst:master Jan 18, 2021
@Martchus Martchus deleted the use-any branch January 18, 2021 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants