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

[bug] test failure with specific seed indicates cross-pollution #2164

Closed
flavorjones opened this issue Jan 4, 2021 · 3 comments
Closed

[bug] test failure with specific seed indicates cross-pollution #2164

flavorjones opened this issue Jan 4, 2021 · 3 comments

Comments

@flavorjones
Copy link
Member

Please describe the bug

The CI test run at https://ci.nokogiri.org/teams/nokogiri-core/pipelines/nokogiri/jobs/cruby-gem-test/builds/66 failed, and a rerun passed. We don't generally have flaky tests, so I investigated.

Thanks to minitest-bisect this was a cinch to narrow down to:

Culprit methods: ["Nokogiri::XML::TestDocument#test_xpath_syntax_error"]
...
# Running tests with run options --seed=59263 -n "/^(?:Nokogiri::XML::TestDocument#(?:test_xpath_syntax_error)|Nokogiri::XML::SAX::TestParserContext#(?:test_from_file))$/":

Help us reproduce what you're seeing

You should be able to repro by running:

TESTOPTS='--seed=59263 "-n/^(?:Nokogiri::XML::TestDocument#(?:test_xpath_syntax_error)|Nokogiri::XML::SAX::TestParserContext#(?:test_from_file))$/"' bundle exec rake test
@flavorjones
Copy link
Member Author

Interestingly, this is fixed now (3d90c6d) and a quick git-bisect shows that this was fixed by 35aa88b.

The first test is Nokogiri::XML::TestDocument#test_xpath_syntax_error which parses an invalid xpath query and asserts on the exception raised. XPathContext#evaluate uses Nokogiri_error_raise as the error handler, and this is bad for a few reasons that I've outlined at #1610, and in fact have fixed on a branch in 426fd89.

One of the reasons Nokogiri_error_raise is bad is that it doesn't give XPathContext#evaluate a chance to reset error handlers, and so we see that the exception is raised leaving the libxml2 global error handler set to `Nokogiri_error_raise.

The second test, which is failing in the description, is Nokogiri::XML::SAX::TestParserContext#test_from_file which parses a document using the SAX parser. As noted in #2168 and #2169, the SAX parser in v1.11.0 does not set an error handler, and so this test "inherits" Nokogiri_error_raise and ... this test fails because the exception is raised.

Closing. Will be fixed in v1.11.1.

@flavorjones
Copy link
Member Author

Oh, and I'm hoping to land fixes for #1610 in the v1.11/v1.12 timeframe.

@flavorjones
Copy link
Member Author

The commit I mentioned above is in a PR sitting at #2096

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

No branches or pull requests

1 participant