Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include additional spec coverages for options param override #42

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kevinlin505
Copy link
Contributor

@kevinlin505 kevinlin505 commented Jan 5, 2025

This pull request includes several changes to the demo_mode feature, focusing on refining the persona generation process and enhancing test coverage. The main updates involve modifying how options are passed to generators and adding new test cases for better validation.

Improvements to persona generation:

  • lib/demo_mode/config.rb: Updated the around_persona_generation method to use a lambda that accepts a generator and options, ensuring options are passed correctly to the generator.

Enhancements to test coverage:

  • spec/demo_mode_spec.rb: Adjusted the around_persona_generation block to accept and pass options, improving test accuracy for the enabled? method.
  • spec/requests/demo_mode/sessions_spec.rb: Added a nested context to test overriding persona generation with around_persona_generation, ensuring the correct handling of options.
  • spec/requests/demo_mode/sessions_spec.rb: Introduced a new test context for overriding persona generation with sign_in_as, validating the creation of sessions and the correct saving of options.

@kevinlin505 kevinlin505 force-pushed the klin/sign-in-as-option-override branch from 73f9903 to 34cbac7 Compare January 5, 2025 19:28
sign_in_as do |**options|
DummyUser.create!(name: options.dig(:example_custom_option, :name) || 'Example Tester')
sign_in_as do |params|
DummyUser.create!(name: params[:options][:example_custom_option][:name])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smudge This way works for passing the options through the callable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a spec-only change, though -- I think we'd want the library to provide a default generator here that expects the options, so that an end developer doesn't need to specify an around_persona_generation in their config:

@around_persona_generation ||= :call.to_proc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'd expect this PR to make some change to the lib/ folder in order to get the test passing.

@kevinlin505 kevinlin505 requested a review from smudge January 6, 2025 20:27
@kevinlin505 kevinlin505 force-pushed the klin/sign-in-as-option-override branch 2 times, most recently from 65d63cc to 27e24f5 Compare January 6, 2025 20:31
@kevinlin505 kevinlin505 force-pushed the klin/sign-in-as-option-override branch 2 times, most recently from 0e166b7 to 7886fac Compare January 6, 2025 21:14
@@ -73,7 +73,7 @@ def around_persona_generation(&block)
if block
@around_persona_generation = block
else
@around_persona_generation ||= :call.to_proc
@around_persona_generation ||= ->(generator, options) { generator.call(options: options) }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smudge Is this what you are referencing to? I can remove the around block with this.

@kevinlin505 kevinlin505 force-pushed the klin/sign-in-as-option-override branch 4 times, most recently from a57d116 to ca208b5 Compare January 7, 2025 03:32
@kevinlin505
Copy link
Contributor Author

@smudge any idea why the specs are failing? I am able to get these pass locally but getting these timeouts here.

@kevinlin505
Copy link
Contributor Author

hmm, @smudge @pat-whitrock I am getting these GitHub action failures even after I reverted my changes as well. Any ideas?

@kevinlin505 kevinlin505 force-pushed the klin/sign-in-as-option-override branch from a675974 to ca208b5 Compare January 7, 2025 21:06
@kevinlin505 kevinlin505 force-pushed the klin/sign-in-as-option-override branch 2 times, most recently from 5b83232 to 802d411 Compare January 8, 2025 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants