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 flaky test #113

Closed
wants to merge 1 commit into from
Closed

Fix flaky test #113

wants to merge 1 commit into from

Conversation

peterzhu2118
Copy link
Member

The test_evaluating_a_variable_entirely_within_c test is flaky. If we do the following:

diff --git a/test/unit/context_test.rb b/test/unit/context_test.rb
index 64b1852..eb5cd93 100644
--- a/test/unit/context_test.rb
+++ b/test/unit/context_test.rb
@@ -44,6 +44,7 @@ class ContextTest < Minitest::Test

       c_call_trace = TracePoint.trace(:c_call) do |t|
         unless t.self == TracePoint || t.self.is_a?(TracePoint)
+          puts "#{t.defined_class} #{t.method_id}"
           called_c_method_count += 1
         end
       end

We can see output such as:

 current
Thread abort_on_exception=
Thread::Queue pop
 current
Thread abort_on_exception=
Thread::Queue pop
Liquid::Context c_evaluate

And the failure is:

  1) Failure:
ContextTest#test_evaluating_a_variable_entirely_within_c [/Users/peter/src/github.com/Shopify/liquid-c/test/unit/context_test.rb:64]:
Expected: 1
  Actual: 7

I was able to reproduce this on Ruby 2.5.8 using seed 7822 (the repro is flaky and requires multiple tries).

As you can see, there are various other methods called that are also C functions.

The solution is to ignore all C function calls unless the class it is called on starts with Liquid.

@peterzhu2118
Copy link
Member Author

Looks like this flaky test is causing the build on master to fail: https://github.com/Shopify/liquid-c/runs/1342582317

(This failure was not introduced in #86, as it can be repro'd on 1dc9459 as well).

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.

1 participant