Generated standard filter tests ignore result type

[dylanahsmith]

The standard library tests in the https://github.com/Shopify/liquid repo are more precise about the result they expect, since they actually compare the ruby objects themselves for equality. Instead, the specs generated from these tests case the result to a string before comparing to the expected cast result string. The casting is done using liquid, which has ambiguities about the actual type given the result of the string cast. For example, nil, "" and [] would all produce an empty string.

[macournoyer]

Do you have a specific example where this would be important? Maybe we could just adjust the test itself to ensure the type is good? All in Liquid.

[dylanahsmith]

Yes, the difference can have an observable effect on liquid, such as in == or != comparisons. This also does mean that it can be tested in liquid itself.

I have a branch where I tried to use an if tag to do the comparison within liquid itself rather than on the result on rendering (fix-generate-standard-filters...check-filter-result). However, that generates tests that don't pass, since the tests duplicate inputs to avoid the code under test mutating them, which also makes test drops that == compares by identity to no longer be equal.

The test generation seems like a quick and dirty way to generate a lot of tests, but it will have trouble with subtleties like that or tests that do things like inspecting state on a drop after the test to make sure to_liquid is called. This would all be more maintainable by making changes to the liquid test suite itself so that it could be re-used for liquid-wasm, similar to how that was done for liquid-c integration testing.

[dylanahsmith]

Looks like we are in fact casting some nil values to strings in filters that preserve nil. This is happening in the truncate, truncatewords and escape filters, which fail the ruby liquid tests, even though they were passing the tests generated from them.

[macournoyer]

The test generation seems like a quick and dirty way to generate a lot of tests, but it will have trouble with subtleties like that or tests that do things like inspecting state on a drop after the test to make sure to_liquid is called. This would all be more maintainable by making changes to the liquid test suite itself so that it could be re-used for liquid-wasm, similar to how that was done for liquid-c integration testing.

Could we, instead of generating new tests from existing ones in this case, extract the tests from Liquid Ruby, and make them generic enough to test all implementations easily? We could still host the test in liquid-spec, but support a new format of specs in Ruby.

[dylanahsmith]

We still want those tests in the liquid repo itself so it still has test coverage while liquid-spec is private. This is especially true given that liquid-spec's mandate seems to be to cover Shopify liquid extensions as well, which would keep it private.

Instead of generating new tests, I'm saying we should just modify the ones in the ruby liquid repo so they aren't coupled to the implementation.