search

rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
809 stars 276 forks source link

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

Open Darhazer opened 7 years ago

Darhazer commented 7 years ago

When someone uses stub_const('Foo::BAZ', 42), all other constants (and methods) defined in Foo may be lost. This is explained in https://github.com/rspec/rspec-mocks/issues/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 commented 7 years ago

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 commented 6 years ago

ping @dgollahon

dgollahon commented 6 years ago

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.