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

Cop idea: detect stub_const that doesn't refer the const #423

Open
Darhazer opened this issue Jun 3, 2017 · 3 comments
Open

Cop idea: detect stub_const that doesn't refer the const #423

Darhazer opened this issue Jun 3, 2017 · 3 comments

Comments

@Darhazer
Copy link
Member

Darhazer commented Jun 3, 2017

When someone uses stub_const('Foo::BAZ', 42), all other constants (and methods) defined in Foo may be lost. This is explained in rspec/rspec-mocks#1079. I've lost couple of hours debugging it the first time it happened to me, and since then few times returned such stubs at a code review. It would be nice to have a cop that warns about this behavior.

@bquorning
Copy link
Collaborator

The issue happens only when Foo does not exist at the time you stub it with stub_const('Foo::BAZ', 42). In this case rspec-mocks does not know anything about Foo other than you need the namespace to define BAZ inside. Thus, it creates Foo as a module.

In my projects, I have a rule of never using nested stubbing without first requiring the “outer” constants. I don’t think we can enforce such a rule with static analysis alone. @dgollahon may know more – perhaps a job for RSpectre?

@bquorning
Copy link
Collaborator

ping @dgollahon

@dgollahon
Copy link
Contributor

I personally avoid this by just requiring all constants upfront.

Regarding rspectre, I think it would be out of scope/possibly difficult to track if the constant previously existed before stub_const. I don't envision rspectre as a tool you run frequently, so I don't think it's the kind of thing that is likely to have saved @Darhazer debugging time. I'm not sure rubocop-rspec would help that much either since an exception is already being raised and i don't know if people use rubocop to lint when they hit a bug frequently and then if they would run all cops rather than just ones categorized as linting cops.

Maybe rspec itself would accept a patch to issue a warning or somehow catch class/module mismatches when the constant is stubbed but the namespace doesn't exist or something like that. I'm really not quite sure what the appropriate response is here.

That said, I think it would be a good addition to rspectre to detect if the stubbed constant is ever accessed, as I imagine a fair number of stubs in the wild are unnecessary. I'll add an issue to that effect.

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

3 participants