-
Notifications
You must be signed in to change notification settings - Fork 574
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
Convert cleanly proxy.t and websocket_proxy.t to use subtests #1520 #1597
Convert cleanly proxy.t and websocket_proxy.t to use subtests #1520 #1597
Conversation
a129c50
to
29db4d5
Compare
is $tx->res->body, 'Hello Hypnotoad!', 'right content'; | ||
}; | ||
|
||
my $shared_tx; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable outside subtest.
@@ -20,110 +20,125 @@ use Mojo::UserAgent; | |||
use Mojo::Util qw(url_escape); | |||
use Mojolicious::Lite; | |||
|
|||
# Silence | |||
app->log->level('fatal'); | |||
subtest "Silence" => sub { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This again. It makes no sense.
|
||
my $test = $dir->child('test.sock'); | ||
my $encoded = url_escape "$test"; | ||
my $daemon; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables outside of subtest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests are fine and could be merged, but others have scoping issues and require more attention.
This pull request is now in conflicts. Could you fix it @spazm? 🙏 |
I'm sorry, but i've decided to close these PRs, because they contain too many converted tests at once. And we don't have the review capacity to re-review the whole thing after every scoping issue fix. |
Rebased away the tests that did not convert cleanly. The PR is updated with 3 files that require only minimal non-whitespace changes. Hopefully this will be less of a review burden. I apologize for the state of this PR last night, I had no idea you'd come read it so quickly! (which is awesome!) |
|
Summary
Convert two tests to use subtests.
proxy.t
: single commented section converted cleanly to subtest. requiring no additional changes.websocket_proxy.t
: converted cleanly from comment blocks into subtests.$kept_alive
,$result
and$ua
needed declaration and/or initialization in the subtests.Motivation
requested in #1520
References
Issue #1520