From db065315ba16458dc84ba32aa12b2e8e20808ad7 Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Tue, 15 Sep 2020 08:36:55 -0400 Subject: [PATCH 1/4] Allow creating symbols that are garbage collected in a test --- test/integration/security_test.rb | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/test/integration/security_test.rb b/test/integration/security_test.rb index 1e55dde..75f0929 100644 --- a/test/integration/security_test.rb +++ b/test/integration/security_test.rb @@ -43,15 +43,22 @@ class SecurityTest < Minitest::Test assert_equal(expected, Template.parse(text).render!(@assigns, filters: SecurityFilter)) end - def test_does_not_add_filters_to_symbol_table + def test_does_not_permanently_add_filters_to_symbol_table current_symbols = Symbol.all_symbols - test = %( {{ "some_string" | a_bad_filter }} ) + # MRI imprecisely marks objects found on the C stack, which can result + # in uninitialized memory being marked. This can even result in the test failing + # deterministically for a given compilation of ruby. Using a separate thread will + # keep these writes of the symbol pointer on a separate stack that will be garbage + # collected after Thread#join. + Thread.new do + test = %( {{ "some_string" | a_bad_filter }} ) + Template.parse(test).render! + nil + end.join - template = Template.parse(test) - assert_equal([], (Symbol.all_symbols - current_symbols)) + GC.start - template.render! assert_equal([], (Symbol.all_symbols - current_symbols)) end From 3dcad3b3cd2e913f6a9ba696c79d997af7551812 Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Tue, 15 Sep 2020 08:46:32 -0400 Subject: [PATCH 2/4] Move test/integration/parse_tree_visitor_test.rb to test/unit The ParseTreeVisitor exposes the liquid internals that won't be kept compatible with liquid-c, so move it out of the integration tests directory so that we can easily ignore it when testing liquid-c --- test/{integration => unit}/parse_tree_visitor_test.rb | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/{integration => unit}/parse_tree_visitor_test.rb (100%) diff --git a/test/integration/parse_tree_visitor_test.rb b/test/unit/parse_tree_visitor_test.rb similarity index 100% rename from test/integration/parse_tree_visitor_test.rb rename to test/unit/parse_tree_visitor_test.rb From 013802c8774e80fbcc78b52a51ce911aad5f25e4 Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Tue, 15 Sep 2020 09:11:38 -0400 Subject: [PATCH 3/4] Move some unit tests without internal coupling to integration tests since I would like to continue supporting these tests in liquid-c in the foreseeable future. --- test/integration/block_test.rb | 44 ++++++++++++++++++ .../context_test} | 2 +- test/integration/tag_test.rb | 45 ++++++++++++++++++ test/unit/block_unit_test.rb | 46 +------------------ test/unit/tag_unit_test.rb | 38 --------------- 5 files changed, 91 insertions(+), 84 deletions(-) rename test/{unit/context_unit_test.rb => integration/context_test} (99%) create mode 100644 test/integration/tag_test.rb diff --git a/test/integration/block_test.rb b/test/integration/block_test.rb index 0120600..5cc2aa4 100644 --- a/test/integration/block_test.rb +++ b/test/integration/block_test.rb @@ -11,4 +11,48 @@ class BlockTest < Minitest::Test end assert_equal(exc.message, "Liquid syntax error: 'endunless' is not a valid delimiter for if tags. use endif") end + + def test_with_custom_tag + with_custom_tag('testtag', Block) do + assert Liquid::Template.parse("{% testtag %} {% endtesttag %}") + end + end + + def test_custom_block_tags_have_a_default_render_to_output_buffer_method_for_backwards_compatibility + klass1 = Class.new(Block) do + def render(*) + 'hello' + end + end + + with_custom_tag('blabla', klass1) do + template = Liquid::Template.parse("{% blabla %} bla {% endblabla %}") + + assert_equal 'hello', template.render + + buf = +'' + output = template.render({}, output: buf) + assert_equal 'hello', output + assert_equal 'hello', buf + assert_equal buf.object_id, output.object_id + end + + klass2 = Class.new(klass1) do + def render(*) + 'foo' + super + 'bar' + end + end + + with_custom_tag('blabla', klass2) do + template = Liquid::Template.parse("{% blabla %} foo {% endblabla %}") + + assert_equal 'foohellobar', template.render + + buf = +'' + output = template.render({}, output: buf) + assert_equal 'foohellobar', output + assert_equal 'foohellobar', buf + assert_equal buf.object_id, output.object_id + end + end end diff --git a/test/unit/context_unit_test.rb b/test/integration/context_test similarity index 99% rename from test/unit/context_unit_test.rb rename to test/integration/context_test index cec5608..85f1b1f 100644 --- a/test/unit/context_unit_test.rb +++ b/test/integration/context_test @@ -65,7 +65,7 @@ class ArrayLike end end -class ContextUnitTest < Minitest::Test +class ContextTest < Minitest::Test include Liquid def setup diff --git a/test/integration/tag_test.rb b/test/integration/tag_test.rb new file mode 100644 index 0000000..3ee1f94 --- /dev/null +++ b/test/integration/tag_test.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'test_helper' + +class TagTest < Minitest::Test + include Liquid + + def test_custom_tags_have_a_default_render_to_output_buffer_method_for_backwards_compatibility + klass1 = Class.new(Tag) do + def render(*) + 'hello' + end + end + + with_custom_tag('blabla', klass1) do + template = Liquid::Template.parse("{% blabla %}") + + assert_equal 'hello', template.render + + buf = +'' + output = template.render({}, output: buf) + assert_equal 'hello', output + assert_equal 'hello', buf + assert_equal buf.object_id, output.object_id + end + + klass2 = Class.new(klass1) do + def render(*) + 'foo' + super + 'bar' + end + end + + with_custom_tag('blabla', klass2) do + template = Liquid::Template.parse("{% blabla %}") + + assert_equal 'foohellobar', template.render + + buf = +'' + output = template.render({}, output: buf) + assert_equal 'foohellobar', output + assert_equal 'foohellobar', buf + assert_equal buf.object_id, output.object_id + end + end +end diff --git a/test/unit/block_unit_test.rb b/test/unit/block_unit_test.rb index 9b3103d..42093ef 100644 --- a/test/unit/block_unit_test.rb +++ b/test/unit/block_unit_test.rb @@ -45,53 +45,9 @@ class BlockUnitTest < Minitest::Test assert_equal(3, template.root.nodelist.size) end - def test_with_custom_tag - with_custom_tag('testtag', Block) do - assert Liquid::Template.parse("{% testtag %} {% endtesttag %}") - end - end - - def test_custom_block_tags_have_a_default_render_to_output_buffer_method_for_backwards_compatibility - klass1 = Class.new(Block) do - def render(*) - 'hello' - end - end - - with_custom_tag('blabla', klass1) do - template = Liquid::Template.parse("{% blabla %} bla {% endblabla %}") - - assert_equal 'hello', template.render - - buf = +'' - output = template.render({}, output: buf) - assert_equal 'hello', output - assert_equal 'hello', buf - assert_equal buf.object_id, output.object_id - end - - klass2 = Class.new(klass1) do - def render(*) - 'foo' + super + 'bar' - end - end - - with_custom_tag('blabla', klass2) do - template = Liquid::Template.parse("{% blabla %} foo {% endblabla %}") - - assert_equal 'foohellobar', template.render - - buf = +'' - output = template.render({}, output: buf) - assert_equal 'foohellobar', output - assert_equal 'foohellobar', buf - assert_equal buf.object_id, output.object_id - end - end - private def block_types(nodelist) nodelist.collect(&:class) end -end # VariableTest +end diff --git a/test/unit/tag_unit_test.rb b/test/unit/tag_unit_test.rb index d72d3b2..b814b1b 100644 --- a/test/unit/tag_unit_test.rb +++ b/test/unit/tag_unit_test.rb @@ -20,42 +20,4 @@ class TagUnitTest < Minitest::Test tag = Tag.parse("some_tag", "", Tokenizer.new(""), ParseContext.new) assert_equal('some_tag', tag.tag_name) end - - def test_custom_tags_have_a_default_render_to_output_buffer_method_for_backwards_compatibility - klass1 = Class.new(Tag) do - def render(*) - 'hello' - end - end - - with_custom_tag('blabla', klass1) do - template = Liquid::Template.parse("{% blabla %}") - - assert_equal 'hello', template.render - - buf = +'' - output = template.render({}, output: buf) - assert_equal 'hello', output - assert_equal 'hello', buf - assert_equal buf.object_id, output.object_id - end - - klass2 = Class.new(klass1) do - def render(*) - 'foo' + super + 'bar' - end - end - - with_custom_tag('blabla', klass2) do - template = Liquid::Template.parse("{% blabla %}") - - assert_equal 'foohellobar', template.render - - buf = +'' - output = template.render({}, output: buf) - assert_equal 'foohellobar', output - assert_equal 'foohellobar', buf - assert_equal buf.object_id, output.object_id - end - end end From 33760f083ab8fe361b96353174a2eaf2815d053e Mon Sep 17 00:00:00 2001 From: Dylan Thacker-Smith Date: Mon, 14 Sep 2020 14:12:19 -0400 Subject: [PATCH 4/4] Extract rescue code from BlockBody#render_node for re-use in liquid-c --- lib/liquid/block_body.rb | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index f27905c..67d5d63 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -71,15 +71,23 @@ module Liquid # @api private def self.render_node(context, output, node) node.render_to_output_buffer(context, output) - rescue UndefinedVariable, UndefinedDropMethod, UndefinedFilter => e - context.handle_error(e, node.line_number) - rescue MemoryError - raise - rescue ::StandardError => e - line_number = node.is_a?(String) ? nil : node.line_number - error_message = context.handle_error(e, line_number) - if node.instance_of?(Variable) || !node.blank? # conditional for backwards compatibility - output << error_message + rescue => exc + blank_tag = !node.instance_of?(Variable) && node.blank? + rescue_render_node(context, output, node.line_number, exc, blank_tag) + end + + # @api private + def self.rescue_render_node(context, output, line_number, exc, blank_tag) + case exc + when MemoryError + raise + when UndefinedVariable, UndefinedDropMethod, UndefinedFilter + context.handle_error(exc, line_number) + else + error_message = context.handle_error(exc, line_number) + unless blank_tag # conditional for backwards compatibility + output << error_message + end end end