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

Convert cleanly proxy.t and websocket_proxy.t to use subtests #1520 #1597

Conversation

spazm
Copy link

@spazm spazm commented Nov 1, 2020

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

@spazm spazm changed the title Fix/1520 5 tests hypnotoad to user agent unix Convert 5 more tests to use subtests #1520 Nov 1, 2020
@spazm spazm force-pushed the fix/1520-5-tests-hypnotoad-to-user_agent_unix branch from a129c50 to 29db4d5 Compare November 1, 2020 09:08
is $tx->res->body, 'Hello Hypnotoad!', 'right content';
};

my $shared_tx;
Copy link
Member

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 {
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

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

Variables outside of subtest.

Copy link
Member

@kraih kraih left a 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.

@mergify
Copy link
Contributor

mergify bot commented Nov 1, 2020

This pull request is now in conflicts. Could you fix it @spazm? 🙏

@kraih
Copy link
Member

kraih commented Nov 1, 2020

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.

@kraih kraih closed this Nov 1, 2020
@spazm spazm changed the title Convert 5 more tests to use subtests #1520 Convert cleanly 3 tests to use subtests: log.t, proxy.t, websocket_proxy.t #1520 Nov 1, 2020
@spazm
Copy link
Author

spazm commented Nov 1, 2020

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

@spazm spazm changed the title Convert cleanly 3 tests to use subtests: log.t, proxy.t, websocket_proxy.t #1520 Convert cleanly proxy.t and websocket_proxy.t to use subtests #1520 Nov 1, 2020
@spazm
Copy link
Author

spazm commented Nov 1, 2020

log.t was already fixed in another branch. removed and rebase to master.

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

2 participants