Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Task to expand test coverage for \Drupal\Component\Utility\Url
.
Comment | File | Size | Author |
---|---|---|---|
#18 | Screen Shot 2013-10-10 at 11.21.41 PM.png | 51.7 KB | Mile23 |
#18 | Screen Shot 2013-10-10 at 11.29.59 PM.png | 50.81 KB | Mile23 |
#17 | url-phpunit-2046245-17.patch | 8.24 KB | jhedstrom |
#17 | interdiff.txt | 716 bytes | jhedstrom |
#13 | url-phpunit-2046245-13.patch | 8.06 KB | Mile23 |
Comments
Comment #1
jhedstromThis patch ups coverage to 93.8%. Currently not sure why tests for
externalIsLocal
are failing, but I've attached those as a diff, as perhaps I'm calling it incorrectly, or perhaps it is legitimately broken.Comment #2
jhedstromAdding missing phpunit tag.
Comment #3
jhedstromOops, #1 had some code from a different patch. The
failing.txt
file there is still relevant foexternalIsLocal()
.Comment #4
dawehnerWow, we are getting better and better on our test coverage!
There is a missing test for externalIsLocal() if we want to cover them all.
Comment #5
jhedstromRe: #4 see the
failing.txt
patch in #1. I'm either mis-callingexternalIsLocal()
, or it is mis-documented, or it is broken. Specifically, calling with these parameters does not return true:'http://example.com/internal/path', 'http://example.com/'
Comment #6
dawehnerYeah this function was never tested, as it just got introduced into Drupal 8.
While running these tests I get a lot of messages which looks like:
Comment #7
Mile23Added a test for when url and base_url are the same in externalIsLocal().
In the process I found a bug. :-) Actually it still had the same result but it never called line 232, so never benefitted from the short-circuit. Reading up on parse_url(), it seems the path will be '/' if the domain name has a trailing '/'.
This is one reason that 100% coverage isn't such a bad idea: You find bugs.
Comment #8
dawehnerThat is why we need this intensive unit tests!
Let's add parameter documentation / return statements.
Comment #9
Mile23The same as #7, minus the bug fix, which now has its own issue: #2076325: Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases.
Comment #10
Mile23Comment #12
Mile23OK, feel kind of stupid here. :-)
Just go back to #6, and I'll put my test additions on the other issue: #2076325: Drupal\Component\Utility\Url::externalIsLocal() is wrong in common cases.
Comment #13
Mile23Started with #6.
@param and @return added.
Removed the test for externalIsLocal(), which will be covered by the other issue.
I think @dataProvider annotation should come last in the docblock, so I put them there. No guidance (yet) from documentation standards.
Comment #14
dawehnerThe actual point in documentation is to explain what these parameters are used for, not to just list the parameters. This is information i can get by just looking at the code.
Comment #15
Mile23I think I left those alone because I didn't understand what was being tested. The rest of the docblocks look better.
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedyes besides this small docs issue i cant see anything else
Comment #17
jhedstromFixes the docblock issues from #14.
Comment #18
Mile23CRAP 163 to 32.
Reviewed and Tested By CRAP.
Comment #19
catchCommitted/pushed to 8.x, thanks!
Comment #21
jhedstrom