search

rubocop / rubocop-factory_bot

Code style checking for factory_bot files.
https://docs.rubocop.org/rubocop-factory_bot
MIT License
47 stars 13 forks source link

FactoryBot/IdSequence autocorrect safe? #131

Closed nroose closed 1 month ago

nroose commented 1 month ago

This is labeled as safe autocorrect? It's definitely not safe for us. Perhaps it is important, and a great idea to adhere to, and maybe if you don't set the start id, it's safe. But it's definitely breaking all the tests it updates for us. We have legacy tests that do set the sequence for the id and use it accordingly. And it would be a bunch of work to re-work that, which we, in our infinite wisdom have decided not to do.

ydah commented 1 month ago

Which cop is the topic about?

nroose commented 1 month ago

Oh, sorry. FactoryBot/IdSequence

pirj commented 1 month ago

Sorry, can you please be more specific? What are your tests like? records = create_list(:record, 10); first = Record.find(1)?

nroose commented 1 month ago

Honestly, I am not really sure why or what is happening. We are getting failures just in the factorybot code itself.

undefined method `id' for an instance of FactoryBot::SyntaxRunner
pirj commented 1 month ago

The cop in question suggests removing the sequence :id, right? Are you saying that this removal is causibg such an error? I’m puzzled, how this can happen?

At this point we’ll need more evidence to take any action. Are you willing to provide a reproduction example in a form of a repository or a runnable self-contained script?

nroose commented 1 month ago

Well, after doing that, I am not sure it's worth it to you, but https://github.com/nroose/autocorrect/blob/main/spec/factories.rb.

pirj commented 1 month ago

I don’t know what issue first_or_initialize is supposed to solve, but it is inherently non-deterministic, and this hints abount non-canonical use of factories.

I suggest you to turn the cop off, or to fix specs to behave deterministically.

pirj commented 1 month ago

My guess is that it’s to implement a fixtures-like aapproach. And to save on the expensive setup before each example. There are better approaches to that, specifically fixtures themselves, or test-prof’s ‘before_all’ and ‘let_it_be’.