Skip to content

Test reporting of liquid error for filter call with wrong number of arguments#1311

Merged
dylanahsmith merged 1 commit into
masterfrom
test-filter-liquid-arg-error
Oct 8, 2020
Merged

Test reporting of liquid error for filter call with wrong number of arguments#1311
dylanahsmith merged 1 commit into
masterfrom
test-filter-liquid-arg-error

Conversation

@dylanahsmith
Copy link
Copy Markdown
Contributor

@dylanahsmith dylanahsmith commented Oct 8, 2020

Depends on the corresponding liquid-c fix (Shopify/liquid-c#76) for CI to pass
Depends on #1310 to fix CI on master

Problem

When trying to use Shopify/liquid-c#59 in Shopify Storefront, I noticed a CI failure that caught a problem with that PR. Specifically, that PR failed to implement the rescue in Liquid::StrainerTemplate#invoke

rescue ::ArgumentError => e
raise Liquid::ArgumentError, e.message, e.backtrace
end

Solution

That rescue is at least needed right now to catch filter calls with the wrong number of arguments, so I added a test for that case. In the future, we may want to make sure filter methods explicitly do this translation for argument error exceptions, so it doesn't hide internal errors.

I added only a partial assertion for the error message, since the number of arguments it says were passed to the method and the number required includes the input argument, which could be confusing for a template author. For example, without the corresponding liquid-c test fix, the error message for | size: 'too many args' is "wrong number of arguments (given 2, expected 1)".

Base automatically changed from only-integration-test-liquid-c to master October 8, 2020 15:52
@dylanahsmith dylanahsmith force-pushed the test-filter-liquid-arg-error branch from fbe86a8 to ba90ba1 Compare October 8, 2020 15:53
@dylanahsmith dylanahsmith merged commit 077bf2a into master Oct 8, 2020
@dylanahsmith dylanahsmith deleted the test-filter-liquid-arg-error branch October 8, 2020 15:55
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.

2 participants