Add definition names to default trait key errors #1421
Conversation
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.
spec/acceptance/traits_spec.rb
Outdated
| expect { FactoryBot.build(:user) }.to raise_error( | ||
| KeyError, | ||
| 'Trait not registered: "inaccessible_trait" ' \ | ||
| '(referenced within "user" definition)' |
There was a problem hiding this comment.
Was this multi-line string formatted by Standard?
Would a HEREDOC be a viable alternative?
There was a problem hiding this comment.
Is this what you were thinking?
<<~MSG.strip
Trait not registered: "inaccessible_trait" (referenced within "user" definition)
MSGAt that point I would prefer:
'Trait not registered: "inaccessible_trait" (referenced within "user" definition)'I didn't write it that way because the line was getting a bit long. But I could be convinced that it is better for readability. What do you think?
There was a problem hiding this comment.
I went with the string all on one line for now, since it is a little bit easier to read.
|
|
||
| def append_definition_name_to_error(error) | ||
| message = "#{error.message} (referenced within \"#{name}\" definition)" | ||
| error.class.new(message).tap do |new_error| |
There was a problem hiding this comment.
This is a clever and effective way to re-raise the underlying KeyError.
Is there a downside to raising an error class that FactoryBot declares, instead of KeyError from Ruby's standard library?
There was a problem hiding this comment.
Sean beat me to this. I don't think it's a bad idea necessarily to use a KeyError but a FactoryBot error class would be sweet considering this seems like a FactoryBot specific error
There was a problem hiding this comment.
Do we consider the originally raised KeyError part of a factory's public interface? Is re-raising the KeyError motivated by backwards compatibility?
There was a problem hiding this comment.
The reason we are using a KeyError rather than a custom FactoryBot error class is because KeyErrors come with Did you mean? suggestions (see b86ec13).
But you are reminding me that I should probably add a test around that case. The Did you mean? message is meant to come at the end, so I might have to insert the custom bit here into the middle somewhere.
There was a problem hiding this comment.
OK, I modified this to handle the Did you mean? message.
There was a problem hiding this comment.
As I think about this more, I think we could probably update the FactoryBot::Registry to raise a custom factory_bot error instead of a KeyError, since by the time we build the new error the Did you mean? message is already populated. But I think that is beyond the scope of this PR. @seanpdoyle or @das3in would either of you be interested in tying that out and making a PR?
There was a problem hiding this comment.
Sure I'll take a stab at that this weekend.
composerinteralia
force-pushed
the
add-definition-to-trait-key-error
branch
from
6025cd5
to
cd7a228
Compare
Closes #1222
Before this commit, referencing a trait that didn't exist would raise a
generic
KeyError: Trait not registered: "trait_name". This cansometime 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
KeyErrorfor missing keys, we rescue that error, then construct a newerror with our custom message using the original error's message and
backtrace.