janko / rodauth-rails

Rails integration for Rodauth authentication framework
https://github.com/jeremyevans/rodauth
MIT License
571 stars 40 forks source link

Add rspec fixtures #123

Closed benkoshy closed 2 years ago

benkoshy commented 2 years ago
janko commented 2 years ago

I would prefer draft pull requests to be used only in rare cases where you would like to get early feedback, in this case it would be easier for me to just receive the PR when it's finished.

benkoshy commented 2 years ago

I'll resubmit

janko commented 2 years ago

I would like to get a fix for this released, so that installing Rodauth doesn't create a test/ directory for RSpec users. Are you still planning on finishing this?

benkoshy commented 2 years ago

Absolutely - am committed to seeing this through after starting it. Got stuck -- hence the delay. If i can't solve it by close of Monday, i will ask for your assistance.

benkoshy commented 2 years ago

@janko I don't know why it's not working. I tried my best. Here is the latest. for some reason, the file isn't copying. perhaps it's because i have misconfiugred the thor actions? (I am unable to re-open the branch).

(If you have any pointers please let me know - doing so may not save you time, but it has the added advantage of developing contributors.)

janko commented 2 years ago

I think hook_for requires you to create a new generator using a certain class naming convention, which would take over from the point where hook_for was called.

Let's use the model generator as an example. As I understood, the entry point is Rails::Generators::ModelGenerator, which declares hook_for :orm. This detects ActiveRecord::Generators::ModelGenerator, which then continues defining the generator commands (which are IMO all public methods), and defines a hook_for :test_framework. This then picks up TestUnit::Generators::ModelGenerator, which defines creating the test file, and declares hook_for :fixture_replacement after that (since creating the test file isn't affected by fixture replacement), but before creating the fixture file, so that it can override that part. When using FactoryBot, FactoryBot::Generators::ModelGenerator gets picked up, which overrides fixture creation with its own implementation.

In our case, if we would use hook_for :test_framework on Rodauth::Rails::Generators::InstallGenerator, I think we'd need to define a new RSpec::Generators::InstallGenerator generator that overrides the fixture creation part. That seems like an overkill, and I think we should only define hook generators for libraries that we own, because in this case we'd be accidentally overriding RSpec's built-in install generator.


So, I recommend using a simple conditional, for example if defined?(RSpec::Rails) inside the generator method. I see you're using a conditional based on config.generators.options[:test_framework], why are you expecting it to be an empty string, isn't it supposed to be :rspec? EDIT: Also, I think it should be options[:rails][:test_framework], at least that's how it is in our app at work.

janko commented 2 years ago

I am unable to re-open the branch

It's probably because yesterday I renamed the master branch to main.

benkoshy commented 2 years ago

This explanation helps point the way - perhaps a little too much ;) - please allow me to digest and finish off this PR tomorrow and day after.

janko commented 2 years ago

Yeah, I could have made the pointers much simpler and more vague. Basically, we don't need hook_for, and re-check your conditional for RSpec 😉

benkoshy commented 2 years ago

@janko I followed your advice, however the files just aren't copying where i would like them to copy. not sure why though.

(I was unable to debug: it refused to stop at the break point).