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

Use subtests t/mojo/template.t; issue #1520 #1914

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tschaefer
Copy link

Summary

Convert t/mojo/template.t to using subtests.

Motivation

See issue #1520

kathackeray
kathackeray previously approved these changes Feb 25, 2022
Copy link
Contributor

@kathackeray kathackeray left a comment

Choose a reason for hiding this comment

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

Tests pass. Same number of total tests/subtests after as before. Tests remain the same.

The $capture string variable is duplicated within 13 subtests with identical content for all.

$ ack 'my \$capture' t/mojo/template.t
  my $capture = 'no warnings "redefine"; sub capture { shift->(@_) }';
  my $capture = 'no warnings "redefine"; sub capture { shift->(@_) }';
  my $capture = 'no warnings "redefine"; sub capture { shift->(@_) }';
  ...

Arguably it could be declared once and reused. It doesn't seem to be subtest-specific and isn't modified. I'm approving the PR because the code is correct and functional. If somebody else fails it on the repetition, I'll know to be stricter next time.

@kraih
Copy link
Member

kraih commented Feb 25, 2022

Arguably it could be declared once and reused. It doesn't seem to be subtest-specific and isn't modified.

In tests it is more important to keep the scenarios isolated than to reduce duplicated code.

@tschaefer
Copy link
Author

Sorry for the confusion. The close was not on purpose.

I just wanted to ask, if I should fit the indentation manually blamed in the perltidy check? The local run of perltidy with the given .perltidyrc doesn't bring the "wanted" result.

@kathackeray
Copy link
Contributor

kathackeray commented Feb 25, 2022

I have version v20211029 of perltidy locally, and running it on that file does produce the indentation changes. Might your version be older?

@tschaefer
Copy link
Author

I'm using v20220217 same as in the related check. It's working from the command line not from within vim, seems some setup stuff from my side. Anyways, I'll force-push the commit.

kathackeray
kathackeray previously approved these changes Mar 9, 2022
Copy link
Contributor

@kathackeray kathackeray left a comment

Choose a reason for hiding this comment

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

Tests pass. Same number of total tests / subtests after as before. Perltidy passes.

Tests remain largely the same. However, I have highlighted a couple of small changes to the test input and expected responses, the reason for which is not explained in the issue as far as I can tell. I doubt this constitutes a fail.

is $output, "<%# 1 + 1 %>\n", 'comment tag has been replaced';
subtest 'Replace comment tag' => sub {
my $mt = Mojo::Template->new;
my $output = $mt->render('<%%# \'1 + 1 %>');
Copy link
Contributor

Choose a reason for hiding this comment

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

Addition of ' to a test input and expected response. Test still passes, but unsure of the reason.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that should not have been added.

Copy link
Author

@tschaefer tschaefer Mar 9, 2022

Choose a reason for hiding this comment

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

I can undo this unintentional adds. To be honest, to do this restructure of the tests is a kind of monkey job. So I tried to do it with some clever regex substitutions, seems that went not that well ... But anyways. I'll go on and try to convert at least one test each week.

is $output, " %# 1 + 1\n", 'comment line has been replaced';
subtest 'Replace comment line' => sub {
my $mt = Mojo::Template->new;
my $output = $mt->render(' %%# \'1 + 1');
Copy link
Contributor

Choose a reason for hiding this comment

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

Addition of ' to a test input and expected response. Test still passes, but unsure of the reason.

subtest 'Multiline comment' => sub {
my $mt = Mojo::Template->new;
my $output = $mt->render(<<'EOF');
<html><%# 'this is
Copy link
Contributor

Choose a reason for hiding this comment

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

Another stray single quote

@mergify
Copy link
Contributor

mergify bot commented Aug 20, 2022

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

4 similar comments
@mergify
Copy link
Contributor

mergify bot commented Apr 27, 2023

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

@mergify
Copy link
Contributor

mergify bot commented Aug 14, 2023

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

@mergify
Copy link
Contributor

mergify bot commented Sep 11, 2023

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

Copy link
Contributor

mergify bot commented Mar 6, 2024

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

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.

3 participants