mojolicious / mojo

:sparkles: Mojolicious - Perl real-time web framework
https://mojolicious.org
Artistic License 2.0
2.67k stars 580 forks source link

Convert tests to using subtests #1520

Open kraih opened 4 years ago

kraih commented 4 years ago

Not a particularly hard task, but converting all tests is a lot of work. We want to go from:

# Promisify
is ref Mojo::Promise->resolve('foo'), 'Mojo::Promise', 'right class';
$promise = Mojo::Promise->reject('foo');
is ref $promise, 'Mojo::Promise', 'right class';
@errors = ();
$promise->catch(sub { push @errors, @_ })->wait;
is_deeply \@errors, ['foo'], 'promise rejected';
$promise = Mojo::Promise->resolve('foo');
is refaddr(Mojo::Promise->resolve($promise)), refaddr($promise), 'same object';
$promise = Mojo::Promise->resolve('foo');
isnt refaddr(Mojo::Promise->new->resolve($promise)), refaddr($promise),
  'different object';
$promise = Mojo::Promise->reject('foo');
is refaddr(Mojo::Promise->resolve($promise)), refaddr($promise), 'same object';
@errors = ();
$promise->catch(sub { push @errors, @_ })->wait;
is_deeply \@errors, ['foo'], 'promise rejected';
$promise  = Mojo::Promise->reject('foo');
$promise2 = Mojo::Promise->reject($promise);
isnt refaddr($promise2), refaddr($promise), 'different object';
@errors = ();
$promise2->catch(sub { push @errors, @_ })->wait;
is_deeply \@errors, ['foo'], 'promise rejected';

to:

subtest 'Promisify' => sub {
  is ref Mojo::Promise->resolve('foo'), 'Mojo::Promise', 'right class';

  $promise = Mojo::Promise->reject('foo');
  is ref $promise, 'Mojo::Promise', 'right class';
  @errors = ();
  $promise->catch(sub { push @errors, @_ })->wait;
  is_deeply \@errors, ['foo'], 'promise rejected';

  $promise = Mojo::Promise->resolve('foo');
  is refaddr(Mojo::Promise->resolve($promise)), refaddr($promise),
    'same object';

  $promise = Mojo::Promise->resolve('foo');
  isnt refaddr(Mojo::Promise->new->resolve($promise)), refaddr($promise),
    'different object';

  $promise = Mojo::Promise->reject('foo');
  is refaddr(Mojo::Promise->resolve($promise)), refaddr($promise),
    'same object';
  @errors = ();
  $promise->catch(sub { push @errors, @_ })->wait;
  is_deeply \@errors, ['foo'], 'promise rejected';

  $promise  = Mojo::Promise->reject('foo');
  $promise2 = Mojo::Promise->reject($promise);
  isnt refaddr($promise2), refaddr($promise), 'different object';
  @errors = ();
  $promise2->catch(sub { push @errors, @_ })->wait;
  is_deeply \@errors, ['foo'], 'promise rejected';
};

Block comments become the subtest description, and then we add a blank line in between unrelated test cases within the subtest. Here is another example.

kraih commented 4 years ago

And you don't have to convert all tests at once, one PR per test file should be fine.

kraih commented 4 years ago

We've converted promise.t in its entirety first, so there is an example for what's expected.

kraih commented 4 years ago

And cgi.t has been converted so there's an example for a test with lite app.

kraih commented 4 years ago

Please keep PRs to one converted test file at a time. We are having issues with reviewing very large patches.

kraih commented 4 years ago

There are a few tests that have a lot of shared state in between blocks. Those will require special consideration, and are probably better skipped unless you want to really dive into the code and understand what exactly was meant to be tested and discuss solutions with the team.

kraih commented 3 years ago

Since we keep linking to the issue, it is probably worth mentioning that all new code should always use subtests. Even if the test file it is added to has not been converted yet.