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

Satisfy all tests on 8.x branch #1075

Open
wants to merge 7 commits into
base: 8.x
Choose a base branch
from

Conversation

sinisterchipmunk
Copy link
Contributor

@sinisterchipmunk sinisterchipmunk commented Apr 12, 2024

I found this conversation ( #1056 ) which led me to the unreleased 8.x branch, but the tests are currently failing.

This pull request makes as few changes as possible in order to make all unit tests pass when running rake test:all from the project root.

The first commit makes some changes that I found necessary in order to connect to Elasticsearch 8.13.2 running in Docker on my local machine. It...

  • disables SSL certificate validation so that the self-signed certificates can be used out-of-the-box
  • changes the startup script to wait for either yellow or green status, as a single ES node is yellow for me under Docker. According to the ES documentation yellow means, "All primary shards are assigned, but one or more replica shards are unassigned." (https://www.elastic.co/guide/en/elasticsearch/reference/current/cluster-health.html) This strikes me as a valid state for a test environment so I included this change in the commit.
  • Fixes an apparently broken reference to CERT_DIR. Since I'm not sure of the history of this constant, I decided to check if it is available using defined?(CERT_DIR) and then only omit the option if the constant is not found, using it as before if it exists.

The second commit actually fixes the tests. There was only one logic change needed.

  • The ClassMethodsProxy needed to use extend on the adapter methods, rather than include. Reverted: see comment, below

I hope that with passing tests we may see a formal 8.x release soon. I am not clear on what else needs to be done but am available to contribute more if needed.

znz and others added 7 commits November 28, 2023 09:47
The goal of ClassMethodsProxy is to avoid polluting the target's namespace,
but it was possible to do this by accident when calling `class_eval` before
ActiveSupport was completely loaded. This test ensures the namespace isn't
polluted, regardless of the load state of ActiveSupport.
ActiveSupport patches Kernel to add `class_eval` but this behavior wasn't
loaded in the test environment. This created a discrepancy between test
and prod, causing tests to fail that should have passed and vice versa.
Fully loading ActiveSupport makes the test environment more accurate.
@sinisterchipmunk
Copy link
Contributor Author

Actually, the problem was more subtle. When the tests were failing, changing from include to extend did make the tests pass, but broke in a real Rails app.

It turns out the call to class_eval was triggering different behavior in test than it was in a real Rails app. In test, class_eval was called on an instance of ClassMethodsProxy, which overrides method_missing and passes the call to class_eval into the inheriting model. Thus calling include as the original code did (before this PR) was including the adapter's methods into the inheriting model, polluting its namespace, while not defining them in the ClassMethodsProxy as intended. The resulting methods in the inheriting model were instance methods, not class methods, and so method_missing couldn't find them when __transform was called at run-time. Changing include to extend made method_missing proxy into the methods, so the tests passed. However, the behavior was still incorrect, because the model's namespace was unintentionally polluted.

In the real application, on the other hand, ActiveSupport defines Kernel#class_eval, so that all objects have this method. But its implementation causes a method to be defined on the singleton class of an instance of an object. So when class_eval was called, this didn't go into ClassMethodsProxy#method_missing at all, but rather called the ActiveSupport implementation. This defined the methods in the right place (the singleton of the ClassMethodsProxy instance), but then extend made them class methods, not instance methods as they needed to be, and broke the real Rails app.

So the solution was to revert the previous commit, changing extend back to include, and then simply ensure ActiveSupport is fully loaded in test with require "active_support/all". This causes the test environment to behave the same as the real Rails app.

Although this was technically all that was needed, I decided to also add a test case to check that the adapter's __transform method isn't erroneously placed into the inheriting model's namespace, since that test might have made this easier to debug, and verifies the correct behavior of ClassMethodsProxy in the first place.

Ironically, this also means no actual business logic change was needed at all, as the test failures were entirely due to the test environment.

@picandocodigo
Copy link
Member

@sinisterchipmunk this is great work, thank you very much. Great find on the class_eval code, and thanks for adding a test. I've incorporated these changes into 8.x and I'll work on a pre-release of elasticsearch-rails 8.0.0 soon. Thanks!

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.

3 participants