Could a core member take a moment to commit these patches?

rubyonrails-core
1

I've got a couple small patches that are quite helpful and minimally invasive that I'd love to get out of my vendor/rails so I can start work on new patches with a clean edge rails (without checking out another copy). If any core member has time to take a look at these and either commit these or give me some feedback I'd really appreciate it:

http://dev.rubyonrails.org/ticket/8683

Koz looked at this one and asked for a test 3 weeks ago, which I provided, but I haven't heard back. This one is particularly useful for development because without it, Template Missing errors don't actually say what template was requested (broken since the addition of multiple controller view paths).

http://dev.rubyonrails.org/ticket/8173

This one's been out there for several months with nary a comment (other than "not so tiny", which I dispute). It makes XML testing with assert_select work. The only condition under which it would break someone's existing tests would be if they were sending content type application/xhtml+xml (which no one does because IE doesn't understand it) AND they were producing invalid XHTML (an unlikely combination to say the least). So it seems like a no-brainer to me. Jamis even blogged about a workaround (Buckblog: assert_xml_select), so it's surprising there's been no comment.

Thanks in advance, Gabe da Silveira

2

I've got a couple small patches that are quite helpful and minimally invasive that I'd love to get out of my vendor/rails so I can start work on new patches with a clean edge rails (without checking out another copy). If any core member has time to take a look at these and either commit these or give me some feedback I'd really appreciate it:

http://dev.rubyonrails.org/ticket/8683

Koz looked at this one and asked for a test 3 weeks ago, which I provided, but I haven't heard back. This one is particularly useful for development because without it, Template Missing errors don't actually say what template was requested (broken since the addition of multiple controller view paths).

Applied. Thanks!

http://dev.rubyonrails.org/ticket/8173

This one's been out there for several months with nary a comment (other than "not so tiny", which I dispute). It makes XML testing with assert_select work. The only condition under which it would break someone's existing tests would be if they were sending content type application/xhtml+xml (which no one does because IE doesn't understand it) AND they were producing invalid XHTML (an unlikely combination to say the least). So it seems like a no-brainer to me. Jamis even blogged about a workaround (Buckblog: assert_xml_select), so it's surprising there's been no comment.

assert_select and xml is way outside my comfort zone so I'll pass on this one. Anyone else have any comments on it?

If you can get three regular contributors to report on the utility of the patch, and the implementation, I'd feel more comfortable applying it.

3

Applied. Thanks!

Thank you!

> http://dev.rubyonrails.org/ticket/8173

assert_select and xml is way outside my comfort zone so I'll pass on this one. Anyone else have any comments on it?

If you can get three regular contributors to report on the utility of the patch, and the implementation, I'd feel more comfortable applying it.

I know there's gotta be people out there bitten by this one. If your XML schema contains auto-closing HTML tags (such as <area>) then testing with assert_select becomes unusable. The patch is so simple, if you're doing RESTful APIs and like the power of assert_select, please take 5 minutes to check it out. It'll save you a lot of ugly hacks.

4

I've been bitten by this before, and would love to see assert_select made more friendly for xml.

- Rob

5

How are css selectors useful when testing xml? Do you just use the element names?

As for the patch, you should probably use the lookup function in mime.rb instead of regexp matching. That'll remove the false positives

6

Gabe da Silveira wrote:

I know there's gotta be people out there bitten by this one.

Yes. The case I got bitten by is the <link> element: in HTML it's self-closing, in feed XML it is not.

I currently use the method from Buckblog: assert_xml_select, which defines a separate assert method for the XML case.

It looks like the original assert_select plug-in may have had something similar in the form of assert_select_feed (it still features in Edge comments, but the code doesn't seem to have survived.)

I don't know of any reason not to fix up the underlying response as in your patch, though. Anyone? I do think the method name seems wrong now - it's not really an html_document you're returning. Should the fix really be applied in response_from_page_or_rjs?

I note that the patch fixes css_select too, as well as assert_select - it currently suffers from the same set of XML problems.

  --Anthony.

7

How are css selectors useful when testing xml? Do you just use the element names?

element names, attributes, child, ancestor, sibling, pseudo-classes like :empty, etc. Pretty much anything but .class and #id selectors are widely useful. Think about testing your XML with regexps (yech!) or installing a parser. assert_select provides an amazingly powerful syntax for a non-trivial problem domain. If only web browsers supported all those selectors!

As for the patch, you should probably use the lookup function in mime.rb instead of regexp matching. That'll remove the false positives

Agreed, this will be an improvement. I'm checking out mime.rb now to figure out how to do this.

8

Okay, I've gone in and made the patch using the lookup function. However, after looking closer at the situation, I believe the original patch is better for a number of reasons which I've detailed in the patch comments.

I don't believe there is any possibilty of a false positive, because the ONLY type that should be parsed as HTML is text/html. Everything else (including XHTML) should be parsed as XML. There are numerous types of XML, including user-defined XML formats, all of which use a standard mime_type convention of application/type+xml, so there is no way to get around the use of a regexp. Meanwhile, the lookup function requires Rails-known mime types, even though the developer can specify any content-type he wants.

So far it seems a few people are interested in the patch. I haven't marked it verified, is the rule for that you have to get the explicit three +1s? Should I go on rails-contrib? I would think the mailing list gets more eyes, but I'm ready to do whatever it takes to get this puppy committed.