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

Add spec for defined? on constant with const_missing #1215

Merged

Conversation

richardboehme
Copy link
Contributor

Make sure that defined? returns nil even if const_missing is implemented in the parent.

We noticed this while implementing defined? in Natalie (see natalie-lang/natalie#2289, natalie-lang/natalie#1734)

@@ -900,6 +900,14 @@
defined?(DefinedSpecs::Undefined).should be_nil
end

it "returns nil when the constant is not defined and the parent implements const_missing" do
Copy link
Member

@andrykonchin andrykonchin Nov 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The term parent is not accurate. It should be about outer scope/module/etc of a constant.

minor: it's common to prefix a method name with either . or # in a spec title so const_missing becomes . const_missing.

end

defined?(DefinedSpecs::Undefined).should be_nil
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dynamic defining a new method in a class/module shared by multiple test cases may make them surprisingly coupled, what makes sense to avoid.

So I would add a new fixture module/class specifically for this test case, e.g. DefinedSpecs::ModuleWithConsMissing instead.

@richardboehme
Copy link
Contributor Author

@andrykonchin Thanks for the review! Actually I've looked at the spec again and there are already tests to check that .const_missing is not being called. I guess the issue why the spec did not fail for us is on Natalie's part then.

The current tests use a should_not_receive expectation (see for example it "does not call .const_missing if the constant is not defined"). Do you see any value in adding a test that actually implements const_missing? Otherwise I think we can close this 😅

@andrykonchin
Copy link
Member

Yeah, I would say it makes sense to add this test case. At least it would catch the issue in your case.

There might be a tiny difference between semantic of the new test and the existing one. Actually the #should_not_receive method does the same - it defines a new method and checks that it isn't called. But its implementation could be changed (e.g. with #method_missing).

Make sure that `defined?` returns nil even if `const_missing` is
implemented in the outer module.
@richardboehme richardboehme force-pushed the rb/add-defined-const-missing-test branch from 4cbc05a to ece2bf4 Compare November 10, 2024 12:16
@richardboehme
Copy link
Contributor Author

Yeah, I would say it makes sense to add this test case. At least it would catch the issue in your case.

There might be a tiny difference between semantic of the new test and the existing one. Actually the #should_not_receive method does the same - it defines a new method and checks that it isn't called. But its implementation could be changed (e.g. with #method_missing).

Ok I've implemented the changes you requested, let me know if anything is missing!

@andrykonchin
Copy link
Member

Thank you! Looks good.

@andrykonchin andrykonchin merged commit 37be2fe into ruby:master Nov 10, 2024
14 checks passed
@richardboehme richardboehme deleted the rb/add-defined-const-missing-test branch November 10, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants