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 dynamic_methods.t, user_agent_socks.t, websocket.t to use subtests #1520 #1596

Closed
wants to merge 5 commits into from

Conversation

spazm
Copy link

@spazm spazm commented Nov 1, 2020

Summary

Convert tests to use subtests:

  • dynamic_methods: single test converted cleanly to subtest.
  • user_agent_socks.t: comment block tests converted cleanly to subtests. Only required rescoping $tx variable.
  • websocket.t: comment block tests converted cleanly to subtests. Several variables required rescoping but there was no data shared between tests: $res, $result, $status, $stash, $code, $ws, $counter $msg.

Motivation

Requests in #1520

References

Requested in #1520

There are a couple of blocks of subtests that don't seem to have tests. They may have just been labeled parts of setting up? Please feel free to revert/modify as necessary.

@spazm spazm force-pushed the fix/1520-next-five-subtests branch from 1c4dfa9 to 2d148f8 Compare November 1, 2020 04:08
@spazm spazm force-pushed the fix/1520-next-five-subtests branch from 3037102 to ab1178a Compare November 1, 2020 07:55
@spazm spazm force-pushed the fix/1520-next-five-subtests branch from ab1178a to 8eea9f3 Compare November 1, 2020 09:09
};

my ($listen, $port) = ();
my ($readable, $writable) = ();
Copy link
Member

Choose a reason for hiding this comment

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

These are outside the subtest.

ok !$text, 'message event has not been emitted yet';
$fragmented_compressed->parse_message([1, 0, 0, 0, WS_CONTINUATION, substr($compressed_payload, 6)]);
is $text, 'just works', 'decoded correctly';
my $dummy;
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 of subtest.

ok !$writable, 'handle is not writable';

# Connect
my $reactor;
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 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.

@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 Fix/1520 next five subtests Convert three tests to subtests: dynamic_methods.t, user_agent_socks.t, websocket.t. #1520 Nov 1, 2020
@spazm
Copy link
Author

spazm commented Nov 1, 2020

Removed the tests that did not convert cleanly. The remaining tests converted cleanly and required only minimal non-whitespace changes to redeclare variables for the new scopes. PR description updated with exact details.

@spazm spazm changed the title Convert three tests to subtests: dynamic_methods.t, user_agent_socks.t, websocket.t. #1520 Convert cleanly dynamic_methods.t, user_agent_socks.t, websocket.t to use subtests #1520 Nov 1, 2020
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