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

fix: tests failing since Twig 3.8.0 #2895

Merged
merged 1 commit into from Jan 26, 2024
Merged

fix: tests failing since Twig 3.8.0 #2895

merged 1 commit into from Jan 26, 2024

Conversation

nlemoine
Copy link
Member

Twig recently changed the way exceptions are thrown during template rendering. Depending on the version, different types of execption can be thrown.

See: twigphp/Twig@85bf01b

Copy link
Member

@Levdbas Levdbas left a comment

Choose a reason for hiding this comment

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

just minor documentation changes. Bloody good job! I was already looking into this as well what caused these failed tests.

tests/test-timber-meta.php Outdated Show resolved Hide resolved
tests/test-timber-meta.php Outdated Show resolved Hide resolved
tests/test-timber-meta.php Outdated Show resolved Hide resolved
@nlemoine
Copy link
Member Author

nlemoine commented Jan 26, 2024

Just banged my head for more than one hour on that bug 🤯

I don't really understand why we are testing this. Checking if those cases (calling {{ object.method_expecting_argument }}) throw exceptions is in Twig scope, not ours.

I feel (already noticed before) we are sometimes testing Twig twice. With the extra burden of supporting a wider range.

@nlemoine nlemoine requested a review from Levdbas January 26, 2024 13:13
@Levdbas
Copy link
Member

Levdbas commented Jan 26, 2024

Just banged my head for more than one hour on that bug 🤯

I don't really understand why we are testing this. Checking if those cases (calling {{ object.method_expecting_argument }}) throw exceptions is in Twig scope, not ours.

I feel there (already noticed before) we are sometimes testing Twig twice. With the extra burden of supporting a wider range.

By lowering the amount of WP and PHP versions we test, hopefully the amount of these kind of tests deprecation will decrease as well.

@nlemoine
Copy link
Member Author

By lowering the amount of WP and PHP versions we test, hopefully the amount of these kind of tests deprecation will decrease as well.

On some other cases, probably.

On that particular one, unless bumping Twig to 3.8 (which we might not want), the problem is that we test things that shouldn't require tests from our side (+ all the other similar tests).

People using Twig should be aware of how Twig handles variables.

Twig recently changed the way exceptions are thrown during template rendering. Depending on the version, different types of execption can be thrown.

See: twigphp/Twig@85bf01b

fix: failing tests for WP <6.4

Add two new methods in base test case to check WordPress version and mark test

Co-authored-by: Erik van der Bas <erik@basedonline.nl>
@nlemoine
Copy link
Member Author

And it's all green again. Merging this to make all other PR pass (or not but not polluted with our own failing tests).

@nlemoine nlemoine merged commit f4a233e into 2.x Jan 26, 2024
29 checks passed
@nlemoine nlemoine deleted the 2.x.-fix-unit-tests branch January 26, 2024 15:32
@gchtr
Copy link
Member

gchtr commented Jan 29, 2024

On that particular one, unless bumping Twig to 3.8 (which we might not want), the problem is that we test things that shouldn't require tests from our side (+ all the other similar tests).

I agree. I guess the idea behind these tests were to check that our custom magic methods work as intended all the way through. I agree with you that we shouldn’t test for argument count errors, though.

People using Twig should be aware of how Twig handles variables.

In my experience, this is something that Timber developers sometimes are not aware of.

@nlemoine
Copy link
Member Author

In my experience, this is something that Timber developers sometimes are not aware of.

Maybe, but is it in our scope? Timber users are in fact also using Twig 🙂

Further more, adding those tests won't bring that behavior knowledge to our end users.

@gchtr
Copy link
Member

gchtr commented Jan 29, 2024

Maybe, but is it in our scope? Timber users are in fact also using Twig 🙂

It isn’t a problem big enough that we should act.

Further more, adding those tests won't bring that behavior knowledge to our end users.

No, I didn’t think these are connected at all. If we should handle the problem in any way, it should be with more extensive documentation, but not with tests.

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

3 participants