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

Refactor how we patch node classes to support driver with browser: :remote #19

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

kratob
Copy link
Member

@kratob kratob commented Jun 17, 2024

As discussed.

Opted to use a less clever mechanisms to detect nested calls of auto_synchronize after all.

@kratob kratob self-assigned this Jun 17, 2024
@kratob kratob requested a review from triskweline June 17, 2024 15:57
lib/capybara-lockstep/capybara_ext.rb Outdated Show resolved Hide resolved
Comment on lines 60 to 64
Nestable.skip_nested_calls(:synchronize_after) do
super(*args, &block)
end
ensure
Lockstep.auto_synchronize
Lockstep.auto_synchronize(log: "Synchronizing after ##{meth}") unless Nestable.nested_call?(:synchronize_after)
Copy link
Member

@triskweline triskweline Jun 19, 2024

Choose a reason for hiding this comment

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

Finde der Kontrollfluss hat sehr viele bewegliche Teile: Nestable + Thread.current + Methode muss 2x mit Nested sprechen.

Wäre hier eine weniger smarte Lösung einfacher nachzuvollziehen? Sowas wie:

def synchronize_after(meth)
  prepend Module.new do
    define_method meth do |*args, &block|
      @synchronize_after_calls ||= 0
      @synchronize_after_calls += 1
      super(*args, &block)
    ensure
      Lockstep.auto_synchronize(log: "Synchronizing after ##{meth}") if @synchronize_after_calls == 1
      @synchronize_after_calls -= 1
    end
  end
end  

Alternativ fände ich es auch gut wenn sich die Lockstep-Klasse um das Halten des "Locks" (@synchronize_after_call) global kümmert.

def synchronize_after(meth)
  prepend Module.new do
    define_method meth do |*args, &block|
      Lockstep.auto_synchronize_after(log: "Synchronizing after ##{meth}") do
        super(*args, &block)
      end  
    end
  end
end  

Copy link
Member Author

Choose a reason for hiding this comment

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

Zweite Lösung find ich nicht so toll, denke dass die Logik nicht in die Lockstep-Klasse reinsollte.

Hab jetzt den ersten Weg umgesetzt, das ist sicher einen Tick einfacher. Wollte vermeiden Instanz-Variablen in fremden Objekten zu hinterlassen, aber glaube es ist so wirklich besser.

@kratob kratob force-pushed the tk/fix-for-remote-driver branch from 9e4b875 to 557692c Compare June 19, 2024 12:57
@kratob kratob force-pushed the tk/fix-for-remote-driver branch from 557692c to 2aa1abc Compare June 19, 2024 12:58
@kratob kratob merged commit 0368485 into main Jun 19, 2024
2 checks passed
@kratob kratob deleted the tk/fix-for-remote-driver branch June 19, 2024 13:50
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