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

Change assert_dom to scope with NodeSet #94

Closed
wants to merge 1 commit into from

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Feb 23, 2021

Change assert_dom to accept a single Nokogiri::XML::Node or a
Nokogiri::XML::NodeSet as an argument without supplementary set of CSS
or XPath selectors.

assert_dom document_root_element.at_css("main") do
  assert_dom "h1", "Hello, World"
end

assert_dom css_select("section") do
  assert_dom "h1"
end

This change is in support of making rails/rails#41291 possible:

require "test_helper"
+ require "capybara/minitest"

class ActionDispatch::IntegrationTest
+  include Capybara::Minitest::Assertions
+
+  def page
+    Capybara.string(current_root_element)
+  end
+
+  def within(*arguments, **options, &block)
+    assert_dom page.find(*arguments, **options).native, &block
+  end
end

@seanpdoyle
Copy link
Contributor Author

seanpdoyle commented Feb 24, 2021

I've responded to the latest feedback, thanks for the review.

I think you've picked up on the subtext, but it might be worth stating outright: I've named the method #within to mimic the Capybara::Session method for the sake of symmetry.

That isn't a technical requirement, though. As an alternative, this change could achieve the same outcome by modifying the assert_dom method to accept a single Node argument.

More methods means a larger interface to maintain into the future, and tweaking existing behavior risks breaking backwards compatibility. @rafaelfranca @kaspth Do either of you have a preference on whether or not we should add a new method to the interface?

@seanpdoyle seanpdoyle changed the title Promote nest_selection to public API as within Extend assert_dom interface to accept a Node or Nodeset without selectors Mar 6, 2021
@seanpdoyle seanpdoyle force-pushed the within branch 2 times, most recently from 07290df to 6d06f8c Compare March 6, 2021 18:55
@seanpdoyle
Copy link
Contributor Author

seanpdoyle commented Mar 6, 2021

@kaspth I've pushed up changes to revert the introduction of #within, and instead change #assert_dom/#assert_select to accept a single node or nodeset argument.

It's currently passing. There is one additional test case I considered adding:

diff --git a/test/selector_assertions_test.rb b/test/selector_assertions_test.rb
index 454a5ab..4f08900 100644
--- a/test/selector_assertions_test.rb
+++ b/test/selector_assertions_test.rb
@@ -227,6 +227,20 @@ class AssertSelectTest < ActiveSupport::TestCase
     assert_dom "aside", text: "other", count: 1
   end
 
+  def test_assert_dom_with_nodeset_and_options
+    render_html %Q{<div><span>foo</span></div><div><span>bar</span></div>}
+
+    assert_dom css_select("div"), count: 2
+    assert_dom css_select("div"), minimum: 1, maximum: 2
+
+    assert_raises Minitest::Assertion do
+      assert_dom css_select("div"), count: 1
+    end
+    assert_raises Minitest::Assertion do
+      assert_dom css_select("div"), minimum: 3
+    end
+  end
+

Unfortunately, I haven't figured out a way to combine a single node/nodeset argument with the keyword arguments, so that case is consistently failing.

In this case, I'm not sure if it's a use case that'll crop up in the real world, and if callers bumped up against it, they could use the CSS selector substitution the way that it currently works. I think what's most risky is that if we omit support for this style of invocation, the keyword arguments are silently ignored, so the assertions would pass as false positives.

If we think supporting that use case is necessary, I might need some assistance from whoever understands the substitution context / CSS selector code to help carry this work across the finish line.

@seanpdoyle seanpdoyle changed the title Extend assert_dom interface to accept a Node or Nodeset without selectors Change assert_dom to scope with NodeSet May 26, 2021
Change `assert_dom` to accept a single `Nokogiri::XML::Node` or a
`Nokogiri::XML::NodeSet` as an argument without supplementary set of CSS
or XPath selectors.

```ruby
assert_dom document_root_element.at_css("main") do
  assert_dom "h1", "Hello, World"
end

assert_dom css_select("section") do
  assert_dom "h1"
end
```

This change is in support of making [rails/rails#41291][] possible:

```diff
require "test_helper"
+ require "capybara/minitest"

class ActionDispatch::IntegrationTest
+  include Capybara::Minitest::Assertions
+
+  def page
+    Capybara.string(current_root_element)
+  end
+
+  def within(*arguments, **options, &block)
+    assert_dom page.find(*arguments, **options), &block
+  end
end
```

[rails/rails#41291]: rails/rails#41291
@seanpdoyle
Copy link
Contributor Author

I've opened rails/rails#43361 to add a public method to hook Capybara integration with ActionDispatch::IntegationTest onto.

I'd rather explore that option than continue to change rails-dom-testing, so I'll close this PR in its current state.

@seanpdoyle seanpdoyle closed this Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants