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.

Sign up for GitHub

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

Jump to bottom

Subtests for mojo ua #1581

Closed
wants to merge 1 commit into from
Closed

Subtests for mojo ua #1581

wants to merge 1 commit into from

Conversation

rabbiveesh
Copy link
Contributor

Summary

Changing the tests for Mojo::UserAgent to use subtests

Motivation

This is a project we're working on

References

as discussed in issue #1520

Currently, I need someone to look this over. I have two tests which started failing and are marked as TODO, and I can't figure out what's going on.

kraih
kraih previously requested changes Oct 24, 2020
View reviewed changes
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.

You forgot to squash commits.

@mergify mergify bot dismissed kraih’s stale review October 24, 2020 19:35

Pull request has been modified.

@rabbiveesh
Copy link
Contributor Author

Squashed. Could you look at the two TODO tests? I don't understand why they aren't keeping alive as expected

kraih
kraih reviewed Oct 26, 2020
View reviewed changes
t/mojo/user_agent.t Outdated Show resolved Hide resolved
kraih
kraih previously requested changes Oct 26, 2020
View reviewed changes
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.

There should be no tests in TODO blocks.

@mergify mergify bot dismissed kraih’s stale review October 26, 2020 13:01

Pull request has been modified.

kraih
kraih previously requested changes Oct 26, 2020
View reviewed changes
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.

Nesting unrelated subtests is a hack, not a solution.

@rabbiveesh
Copy link
Contributor Author

Nesting unrelated subtests is a hack, not a solution.

Would you prefer if I removed the assertions that depend on unrelated state? Or should the $ua be scoped to the file, and reinstantiated when necessary?

@kraih
Copy link
Member

kraih commented Oct 26, 2020

Neither, the test should be fixed up so coverage stays the same and the code is clean.

@mergify
Copy link
Contributor

mergify bot commented Oct 26, 2020

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

@mergify mergify bot dismissed kraih’s stale review October 27, 2020 13:22

Pull request has been modified.

@rabbiveesh
Copy link
Contributor Author

Okay, so I switched the assertions to reflect where the ua got reinstantiated. Now there are no sub-subtests.

@jberger
Copy link
Member

jberger commented Oct 27, 2020

Needs a tidy.

jberger
jberger previously approved these changes Oct 27, 2020
View reviewed changes
@mergify
Copy link
Contributor

mergify bot commented Oct 27, 2020

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

jhthorsen
jhthorsen requested changes Oct 28, 2020
View reviewed changes
Copy link
Member

@jhthorsen jhthorsen left a comment

Choose a reason for hiding this comment

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

Using https://github.com/mojolicious/mojo/pull/1581/files?w=1 I see some test changes that I don't think is acceptable. See inline comments.

t/mojo/user_agent.t
@@ -69,16 +69,14 @@ get '/redirect_close' => sub {
$c->res->fix_headers->headers->remove('Content-Length');
};

# Max redirects
{
subtest 'Max redirects' => sub {
local $ENV{MOJO_MAX_REDIRECTS} = 25;
is(Mojo::UserAgent->new->max_redirects, 25, 'right value');
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason for the parenthesis here? I know it's not directly related to the PR, but I was wondering if this should be removed as well to make the syntax more consistent.

t/mojo/user_agent.t
is $tx->res->body, 'works!', 'right content';
};

subtest 'Again' => 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 is no good: I think the idea here is to make sure that "kept_alive" is true between requests. Not quite sure how to format that though. Maybe a $ua_keep_alive object outside of the subtests? Or maybe a "# Again" comment inside the subtest? I guess the latter is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll tell you, I can't tell how much of these tests are actually testing an edge case, or if they were cargo culted from a previous test. So I have no idea what is affecting code coverage here or not.
All of these tests were subtly using the previous state of unrelated tests, which would lead me to believe that they were more boilerplate than real test. But I don't know. I could change it to gratuitous second requests in each of the tests that had to be changed, if that's necessary. But I don't know if it's just noise, as above.

Copy link
Member

Choose a reason for hiding this comment

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

Keep-alive tests tend to be significant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so should I add an extra call wherever there was a keep-alive test? My original thought was that kept-alive mirrors keep-alive, so I can just check one of the two.

t/mojo/user_agent.t
ok $tx->is_finished, 'transaction is finished';
ok $tx->res->is_finished, 'response is finished';
ok !$tx->error, 'no error';
ok !$tx->kept_alive, 'kept connection not alive';
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is acceptable. Same goes for the other kept_alive/keep_alive changes. But maybe it's ok if the "Again" test above is fixed?

t/mojo/user_agent.t
$stream += @{Mojo::IOLoop->stream($tx->connection)->subscribers('drain')};
subtest 'Introspect' => sub {
my $ua = Mojo::UserAgent->new(ioloop => Mojo::IOLoop->singleton);

Copy link
Member

Choose a reason for hiding this comment

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

Extra newline.

@kraih
Copy link
Member

kraih commented Oct 30, 2020 •
edited

Since tests are actually being changed here please make sure to do an in depth review. So we can be sure coverage doesn't change.

@mergify
Copy link
Contributor

mergify bot commented Dec 5, 2020

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

@kraih
Copy link
Member

kraih commented Dec 5, 2020

I'll close this PR for now, so it doesn't block someone else from converting the test.

@kraih kraih closed this Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraih kraih kraih left review comments

@jhthorsen jhthorsen jhthorsen requested changes

@jberger jberger jberger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rabbiveesh @kraih @jberger @jhthorsen