Context helpers for callbacks:

[CodeMeister]

Fixes: #1694
Fixes: #1649

Context is provided as a block parameter in callbacks. This pull requests adds some helper methods to context, making it easier to detect the strategy used and identify which attributes were defined by the user / factory.

Methods included are:

• :stategy
• :defined_attributes
• :user_defined_attributes
• :factory_defined_attributes

All can be queried:

context.strategy         #=> 'build'
context.strategy.build?  #=> true

context.user_defined_attributes           #=> [:age, :name]
context.user_defined_attributes.age?      #=> true
context.user_defined_attributes.height?   #=> false

Although the Evaluator class is marked @api private, being included as a callback parameter means users are already accessing it. Making it easier to use can only be a good thing.

Tests and documentation added.

[CodeMeister]

Hi @sarahraqueld & @smaboshe, any chance of reviewing the pull request?

I have another request ready to go, but it uses some of the same code from this one, so need this one approved (or rejected) first.

😄

[unused]

The PR is great work @CodeMeister very nice extension to the feature set. However, I wonder what use case(s) does justify the exposure of that information? I can't think of one and never encountered problems in that way (afair). Do you think there is a risk of encouraging developers to come up with adding unnecessary complexity to their factories exposing such interface? If there's a couple of real world examples to follow I think it would be much easier to decide.

[CodeMeister]

Thanks @unused.

I don't think this encourages developers to create more complex factories. Most developers know what they need their factories to do, then hunt for the means to make it work.

If a developer needs to know the build strategy used, they will check the docs first, then google it and get context.instance_variable_get(:@build_strategy).to_sym in the results.

I think simplifying access to a factory's internal data & settings can only be a good thing; provided it doesn't change, or allow the user to change, the underlying information. Maintenance considerations should be minimal as these methods will only ever change if the underlying factory structure changes.

As for real-world examples.

(1)

I often improve the performance of an object's unit tests, by stubbing its associations when it was being built, and acting normally otherwise.

I appreciate mocking and stubbing would be even faster, but then you don't catch any changes to the associated objects.

(2)

When running a complex system test, I have to pre-populate the application with '000s of records. When an unexpected value appears in the generated data, the easiest way to track it down is to create a log file of every factory called, with the object's id, specific attributes and strategy used. Skipping a log entry on after(:build) for objects being created reduces this list significantly.

(perhaps I should add an after(:all) callback that only runs once on completion)

😃

[unused]

Thank you a lot for adding that many details. Looking at the two examples I don't think there is a general case where the context build is needed and this rather solving edge cases or optimizations. However, I don't see any disadvantages when adding it. When it comes to the suggested changes, the documentation you added is definitely helpful 🚀 My suggestion would be to not add inquiry, remove attributes defined methods, as well as replace output-check in tests (e.g. just assigning values to an array and check contents). What do you think about that?

[CodeMeister]

Interesting thought.

As this request is just syntactic sugar, perhaps it represents a bigger decision that ThoughtBot needs to make. Rather than focussing on the helpers in this specific request, it should be a more general strategic decision on whether developers should, or should not, have easy access to a factory's internal data & settings.

As you said earlier, easy access will allow developers to create more complex factories and require more code/maintenance. Restricting access will mean less code/maintenance and have developers write their own helpers methods (when needed), with possible unforseen consequences for them.

If the decision is to encourage easy access, then context.strategy is more friendly than context.instance_variable_get(:@build_strategy).to_sym, and context.strategy.build? is more friendly than context.strategy == :build

If not, then contributors will know to avoid this type of syntactic sugar.

As to the tests: we are creatures of habit. My first thought was to parse the captured output, but you are quite correct that adding to an array would be just as effective.

@smaboshe & @sarahraqueld, does ThoughtBot have a policy or opinion on this type of issue? If not, how about setting up workshop like the 'OpenSource day' where a we could discus the issue and reach a consensus?

🤔

[CodeMeister]

@unused, on a separate note, I've submitted a pull-request (#1709) for the <attribute> & <attribute>_id conflict we look at during the Open Source day. I'd value your thoughts on it if you have the time, or inclination. 🫶

[CodeMeister]

Updated the tests to remove the use of :std_out by recording the results within the object itself.

Great idea @unused