* Add definition names to default trait key errors
Closes#1222
Before this commit, referencing a trait that didn't exist would raise a
generic `KeyError: Trait not registered: "trait_name"`. This can
sometime make it difficult to know where exactly the error is coming
from. The fact that implicitly declared associations and sequences will
fall back to implicit traits if they can't be found compounds this
problem. If various associations, sequences, or traits share the same
name, the hunt for the error can take some time.
With this commit we include the factory or trait name (i.e. the
definition name) in the error message to help identify where the
problematic trait originated.
Because trait lookup relies on a more generic class that raises a
`KeyError` for missing keys, we rescue that error, then construct a new
error with our custom message using the original error's message and
backtrace.
Fixes#1411
Before this commit, anybody with Active Record enum attributes using
factory_bot 6.0 would have seen an increase in memory every time they
used any trait.
This behavior can be seen with the following benchmark:
```rb
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
gem "factory_bot", "~> 6.0.1"
gem "activerecord"
gem "sqlite3"
end
require "active_record"
require "factory_bot"
require "logger"
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Schema.define do
create_table :posts, force: true do |t|
t.integer :status
end
end
class Post < ActiveRecord::Base
enum status: ("a".."z").to_a
end
FactoryBot.define do
factory :post do
trait :x
end
end
require "benchmark"
TIMES = 50
FactoryBot.build(:post)
Benchmark.bm do |bm|
bm.report("false, false") do
FactoryBot.automatically_define_enum_traits = false
TIMES.times do
FactoryBot.build(:post)
end
end
bm.report("true, false") do
FactoryBot.automatically_define_enum_traits = true
TIMES.times do
FactoryBot.build(:post)
end
end
bm.report("false, true") do
FactoryBot.automatically_define_enum_traits = false
TIMES.times do
FactoryBot.build(:post, :x)
end
end
bm.report("true, true") do
FactoryBot.automatically_define_enum_traits = true
TIMES.times do
FactoryBot.build(:post, :x)
end
end
end
```
The first three cases work as expected, but in the last case, with
` FactoryBot.automatically_define_enum_traits = true` and the `:x` trait
passed in at run time, the behavior before this commit was as follows:
1. When passing in traits, the [factory runner] calls
[factory.with_traits], which clones the factory and then applies the
traits to the clone.
2. Cloning the factory also clones the factory's definition
3. The cloned definition retains some of the state of the original
definition, most notably [`@defined_traits` and
`@registered_enums`][shared state], but it sets `@compiled` back to false.
4. When the [definition recompiles], it [re-registers enums] into
`@register_enums`, duplicating what is already there. For each
registered enum, it [adds the relevant traits] to @defined_traits,
again duplicating what was already there.
5. This gets worse and worse every time a trait gets used, increasing
the size of `@defined_traits` exponentially until the program grids
to a halt.
With this commit, we keep an additional piece of state to ensure we only
register and apply enum traits in the original definition. When any
cloned definitions recompile, they will skip these steps.
[factory runner]: 0785796f08/lib/factory_bot/factory_runner.rb (L16-L18)
[factory.with_traits]: 0785796f08/lib/factory_bot/factory.rb (L93-L97)
[shared state]: 0785796f08/lib/factory_bot/definition.rb (L10-L11)
[definition recompiles]: 0785796f08/lib/factory_bot/definition.rb (L47-L49)
[re-registers enums]: 0785796f08/lib/factory_bot/definition.rb (L146-L148)
[adds the relevant traits]: 0785796f08/lib/factory_bot/definition.rb (L150-L153)
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>
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 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 [#1259]
Fixes [#1267]
In [#1156] we added support for looking up local traits with either a
symbol or string. This matches the lookup for factories and for global
traits.
We were calling `to_sym` on the trait arguments passed in when
building an object, assuming that those arguments would always respond
to `to_sym`. This leads to a confusing error message when passing an
object that doesn't respond to that method.
This PR replaces the confusing "NoMethodError: undefined method `to_sym`
for ..." with a more helpful "KeyError: Trait not registered: ...". It
ensures that all trait resolution is done using strings instead of
symbols, all objects should have a `to_s` method.
Co-authored-by: Sean Doyle <sean.p.doyle24@gmail.com>
[#1259]: https://github.com/thoughtbot/factory_bot/issues/1259
[#1267]: https://github.com/thoughtbot/factory_bot/issues/1267
[#1156]: https://github.com/thoughtbot/factory_bot/pull/1156
* Alphabetize gem listing in various Gemfiles [Rubocop Bundler/OrderedGems]
* Fix alignment of if/else/end statement [Rubocop Layout/ElseAlignment]
* Method definitions should have a empty line between them [Rubocop Layout/EmptyLineBetweenDefs]
* Modules, Classes, and blocks should have an empty line around them [Rubocop]
Cops:
Layout/EmptyLinesAroundBlockBody
Layout/EmptyLinesAroundModuleBody
Layout/EmptyLinesAroundClassBody
Layout/EmptyLinesAroundAccessModifier
* Keep a blank line before and after access modifiers [Rubocop Layout/EmptyLinesAroundAccessModifier]
* Remove misc extra whitespace [Rubocop Layout/ExtraSpacing]
* Indent the first line of the right-hand-side of a multi-line assignment [Rubocop Layout/IndentAssignment]
* Remove extraneous whitespace [Rubocop]
Cops:
Layout/IndentationWidth
Layout/LeadingCommentSpace
Layout/SpaceAroundEqualsInParameterDefault
Layout/SpaceInsideArrayLiteralBrackets
Layout/SpaceInsideBlockBraces
Layout/SpaceInsideParens
Layout/TrailingBlankLines
* Revert rubocop changes to gemfiles; exclude files from rubocop checks
The files in gemfiles/ are generated by Appraisal, so we shouldn't edit them. Instead, let's tell RuboCop to exclude this directory.
This was originally opened as #1078, but this addresses the review
comments on that PR.
By registering the inline sequences, we allow them to get rewound with
`FactoryBot.rewind_sequences`. We register them with
`__#{factory_name}_#{sequence_name}__` to avoid conflicting with any
reasonably named global sequences, and to hint that we should not be
generating values from these sequences directly.
Co-authored-by: Damian Le Nouaille <dam@dln.name>
Co-authored-by: Damian Galarza <galarza.d@gmail.com>
