Before this commit, factory_bot registered default callbacks in a global
registry, and offered a `register_callback` method for registering any
custom callbacks (which would be fired within custom strategies).
The idea here seems to have been that we could check against this global
registry and then raise an error when referring to a callback that was
not registered.
However, nobody ever saw this error because we have always had the line:
`FactoryBot::Internal.register_callback(name)`, which registers
callbacks on the fly whenever they are referred to. (I noticed this when
a [change to default callbacks] accidentally duplicated `after_create`
instead of including `before_create`, and we didn't get any tests
failures.)
We have two options here:
1. Preserve the existing behavior by deleting all the callback
registration code. (That is what this commit does)
2. Change to the code try and capture what I assume was the original
intention. This would be a breaking change, so we would want to
introduce it in a major release. (See #1379)
I prefer preserving the existing behavior because we haven't seen any
issues around this, and making a breaking change for this doesn't seem
worthwhile.
[change to default callbacks]: f82e40c8c5 (diff-c6d30be672f880311a7df0820dc4fb21R12-R14)
## Enum traits
Given a Rails model with an enum attribute:
```rb
class Task < ActiveRecord::Base
enum status: {queued: 0, started: 1, finished: 2}
end
```
It is common for people to define traits for each possible value of the enum:
```rb
FactoryBot.define do
factory :task do
trait :queued do
status { :queued }
end
trait :started do
status { :started }
end
trait :finished do
status { :finished }
end
end
end
```
With this commit, those trait definitions are no longer necessary—they are defined automatically by factory_bot.
If automatically defining traits for enum attributes on every factory is not desired, it is possible to disable the feature by setting `FactoryBot.automatically_define_enum_traits = false` (see commit: [Allow opting out of automatically defining traits](5a20017351)).
In that case, it is still possible to explicitly define traits for an enum attribute in a particular factory:
```rb
FactoryBot.automatically_define_enum_traits = false
FactoryBot.define do
factory :task do
traits_for_enum(:status)
end
end
```
It is also possible to use this feature for other enumerable values, not specifically tied to ActiveRecord enum attributes:
```rb
class Task
attr_accessor :status
end
FactoryBot.define do
factory :task do
traits_for_enum(:status, ["queued", "started", "finished"])
end
end
```
The second argument here can be an enumerable object, including a Hash or Array.
Closes#1049
Co-authored-by: Lance Johnson <lancejjohnson@gmail.com>
Co-authored-by: PoTa <pota@mosfet.hu>
Co-authored-by: Frida Casas <fridacasas.fc@gmail.com>
Co-authored-by: Daniel Colson <danieljamescolson@gmail.com>
Closes#1336
* removes all the deprecated methods
* removes Ruby 2.3, 2.4 and Rails 4.2 from travis
* bundle updates the test gemfiles
* Removes some pre-5.0 logic from a test helper
* Targets Ruby 2.5 with rubocop and fixes violations
We could also remove support for Rails 5.0 and 5.1, which are now EOL,
but I don't see a strong reason to do that. We don't seem to have to do
anything special to support those versions.
Some codebases have long builds that create over 1000 records, and may
have id collisions between records created with FactoryBot.create and
FactoryBot.build_stubbed
Co-authored-by: Daniel Colson <danieljamescolson@gmail.com>
This commit deprecates several undocumented top-level FactoryBot
methods:
* callbacks
* configuration
* constructor
* initialize_with
* register_sequence
* resent_configuration
* skip_create
* to_create
factory_bot users are not meant to access these methods directly.
Instead they are available for internal use through the new Internal
module introduced in #1262.
---
While addressing the new deprecation warnings, I tried to make the
delegation a bit more consistent, always delegating to
`FactoryBot::Internal`.
This involved requiring "factory_bot/internal" sooner. To avoid having
to fight with the order of requires and potential circular dependencies,
I moved the default strategy and callback registration out of constants
(this is how they used to be before #1297), so it doesn't matter if
those strategies are loaded before the `Internal` module. I also deleted
a pair of tests that were using these constants - these strategies and
callbacks are covered elsewhere in our test suite, and I don't think
these tests were adding much value (they missed #1379, for example).
Why:
These methods are used internally for the functionality of the library
and are subject to change. Therefore shouldn't be part of the public
interface.
This PR:
- Moves the ```register_strategy```, ```register_callback```,
```register_default_factories```, ```register_default_callbacks```
```strategies```, ```callback_names```
and ```strategy_by_name``` methods to the ```FactoryBot::Internal```
class.
- Deprecates the use of ```register_callback``` from the ```FactoryBot```
module.
Why:
These methods are used internally for the functionality of the library
and are subject to change. Therefore shouldn't be part of the public
interface.
This PR:
- Moves the ```register_factory``` and ```factory_by_name``` methods to
the ```FactoryBot::Internal``` class.
- Deprecates the use of ```register_factory``` and ```factory_by_name```
from the ```FactoryBot``` module.
- Improve formatting of the specs
Why:
Another run of internal methods that should not be publicly
available from the base namespace.
This time the sequence involving methods were moved. It's worth noticing
that the ```Internal``` class is getting crowded. Maybe we can start
name-spacing the internal groups into modules under
```internal/sequence.rb``` ```internal/trait.rb``` and so on. Thoughts?
This PR:
- Moves the ```register_sequence```, ```sequence_by_name```,
```sequences``` and ```rewind_sequences``` to the
```FactoryBot::Internal``` module.
- Deprecates uses of ```sequence_by_name```, and ```sequences```
from the ```FactoryBot``` module.
- Refactor rewind sequences test to use spies
This is one of the steps towards fixing [this
issue](https://github.com/thoughtbot/factory_bot/pull/1285#1281)
Why:
These are essentially internal methods that should not be publicly
available from the base namespace.
One thing worth noticing is that the use of this methods internally was
almost exclusively in the `syntax/default` except for one use on the
`factory_bot/definition`.
Also the deprecation silencing module was referring to the singleton
instance of the ```ActiveRecord::Deprecation``` class and not to the new
deprecation instance that was being used in the ```FactoryBot``` module.
This PR:
- Moves the `trait_by_name` and `register_trait` into the
`FactoryBot::Internal` module
- Deprecates uses of `trait_by_name`, `register_trait` and `traits` from
the `FactoryBot` module.
- Rename DEPRECATOR => Deprecation
This is one of the steps towards fixing [this
issue](https://github.com/thoughtbot/factory_bot/issues/1281)
Fixes#1257
When sequence rewinding was first introduced in #1078 it only applied to
globally defined sequences. To get rewinding to work for inline
sequences as well we registered them "privately" in the global registry
in #1164. Unfortunately in #1164 we did not take inline sequences inside
traits into consideration. Since trait names are not unique, it is
possibly to get a `FactoryBot::DuplicateDefinitionError` when defining
two sequences that have the same name in two traits that have the same
name.
This PR abandons the idea of "privately" registering inline sequences,
and instead keeps a separate list of all the inline sequences just for
the purpose of rewinding them.
The `FactoryBot` module has a mixture of methods that are meant for use
by people using the library, and methods that are meant only for
internal use. The methods meant for internal use are cluttering the
documentation, and may be confusing to users.
This change was prompted by [#1258]. Rather than introduce yet another
public method on `FactoryBot` meant only for internal use,
we can introduce a `FactoryBot::Internal` module,
and avoid generating documentation for that module.
The `FactoryBot::Internal.register_inline_sequence` method in [#1258]
will need access to the configuration instance, so this PR moves that
into `FactoryBot::Internal`. Eventually I plan to deprecate
`FactoryBot.configuration` and `FactoryBot.reset_configuration`, and to
move more of the `FactoryBot` methods into `FactoryBot::Internal`, but I
would rather hold off on all that until the dust settles on the 5.0
release.
[#1258]: https://github.com/thoughtbot/factory_bot/pull/1258
The `default:` option of `mattr_accessor` wasn't added until Rails 5.2,
which means it has no effect on older versions and the default value of
`FactoryBot.use_parent_strategy` in 5.0.0.rc1 is nil on those versions.
I removed allow_class_lookup entirely in 2e0b476 when removing the
deprecated ability to lookup factories by class. But the upgrade to
fb 5 will be a bit more smooth if we deprecate the method first.
Background
---
In issues #64 and #66 (from 2010) people expressed surprise
that using the `build` strategy would still `create` any associations.
I remember being similarly surprised by that behavior when I first
started using factory_bot.
The main reason for this behavior is to ensure the built instance will
be valid if there were any foreign key validations
(like `validates_presence_of :user_id`). If we don't save the
associations, there won't be any ids present.
PR #191 (from 2011) offers a workaround for people who don't want
records to be saved automatically. Passing `strategy: :build`
(originally `method: :build`, but later renamed) when declaring the
association will prevent it from being saved when using `build`.
But #191 isn't really a complete solution (as discussed on the PR
after it was merged). `strategy: :build` may do
the right thing when building objects with the `build` strategy, but it
can cause new problems when using the `create` strategy.
A better solution would be something like:
`strategy: <whatever strategy I was already using>`.
PRs #749 and #961 (merged in 2016) introduce something like that,
with the `use_parent_strategy` configuration option.
With this turned on `build` end up being generally [a little faster][]
than `build_stubbed`, since it no longer needs to hit the database for
each association.
[a little faster]: https://gist.github.com/composerinteralia/d4796df9140f431e36f88dfb6fe9733a
I have set `use_parent_strategy` on several projects now. I also added
it to suspenders in thoughtbot/suspenders#952.
On newer projects I have not run into any problems. On existing
projects I have seen the occasional test failure, which are easy enough
to fix by changing `build` to `create`.
Unfortunately I don't think `use_parent_strategy` is widely known,
since it wasn't documented until #1234 (about a month ago).
I also learned in #1234 that the `use_parent_strategy` setting gets
wiped out with `FactoryBot.reload`, which can be problematic when using
factory_bot_rails.
To summarize, we have been exploring having a `build` strategy that uses
`build` all the way down since 2010, but there still isn't a totally
reliable way to get that.
This PR
---
* Default to using the parent strategy for factory_bot 5, as suggested
in #749.
* In the original PR for this (#1240) I had removed
`use_parent_strategy`, but to make the transition smoother I ended up
fixing the reload problem in #1244
Once factory\_bot 5 is released and the dust has settled, it may make
sense to deprecate `use_parent_strategy`, and maybe also the strategy
option for associations.
Before this PR, `use_parent_strategy` was set on the configuration
instance. Since `FactoryBot.reload` wipes out the configuration, it also
ends up resetting `use_parent_strategy` back to `nil`.
This can cause problems when using factory_bot_rails with Spring, since
it calls `FactoryBot.reload` each time Spring forks. If
`use_parent_strategy` is set in a file that Spring preloads, like in an
initializer, then the value will get wiped out.
With this PR, we set `use_parent_strategy` directly on FactoryBot,
rather than on the configuration instance, and so reloading no longer
has any effect.
This has come up a few times, and I can see why it might be helpful to
have access to full backtraces when debugging a factory error uncovered
by `FactoryBot.lint`. But since most of the time I don't want the extra
noise from the backtrace, I added this as a verbose option.
The default message is still:
```
The following factories are invalid:
* user - undefined method `save!' for #<User:0x00007ff0cbc89100>
* admin - undefined method `save!' for #<User:0x00007ff0cbc73e40>
```
And with the verbose option (usually with more lines of backtrace):
```
The following factories are invalid:
* user - undefined method `save!' for #<User:0x00007ff0cbc89100>
/Users/.../thoughtbot/factory_bot/lib/factory_bot/evaluation.rb:18:in `create'
/Users/.../factory_bot/lib/factory_bot/strategy/create.rb:12:in `block in result'
* admin - undefined method `save!' for #<User:0x00007ff0cbc73e40>
/Users/.../thoughtbot/factory_bot/lib/factory_bot/evaluation.rb:18:in `create'
/Users/.../factory_bot/lib/factory_bot/strategy/create.rb:12:in `block in result'
```
I moved the linting option defaults out of the FactoryBot.lint method
and into keyword argument defaults in Linter#initialize. This seems a
bit cleaner, and now we will get an error if we pass an option we don't
understand (before 6e511597 we had a test that passed in a bogus
option)
Closes#710Closes#1124
I am opening a new PR since the original PR is years old and it seemed
unkind to request changes after so long. Instead I will list the authors
as co-authors.
Co-authored-by: Jack Kinsella <jack.kinsella@gmail.com>
Co-authored-by: Jasper Woudenberg <mail@jasperwoudenberg.com>
This refactoring work should make adding the verbose linting option
(started in PR #1233) fairly simple.
I moved the linting option defaults into keyword argument defaults for
Linter#initialize. With keyword arguments we will now get an error if we
pass an option the Linter doesn't understand (we were doing exactly this
in one of our tests before 6e511597). Adding the verbose option will now
be as simple as adding another keyword argument, then adding backtraces
to the error messages when the option is set to true.
Closes#1196
We deprecated looking up factories by class in #877. We introduced
`allow_class_lookup` option so that people could disable the behavior
entirely after fixing the deprecation warning.
In preparation for factory_bot 5, I am removing the deprecation warning
and the `allow_class_lookup` option. It is no longer possible to look
up factories by class. This has also made the ClassKeyHash decorator
unnecessary. The behavior is technically a little different now
that we are using HashWithIndifferentAccess
instead of calling to_sym on the key, but it
should behave identically for any standard,
documented factory_bot usage.
