TestingParty08: external URL options need a test
chx - August 17, 2008 - 08:10
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
Assert that calling url() with an external URL
- containing a fragment works with and without a fragment in $options.
- containing or not containing a query works with a query in $options.
#1
Working on it..
#2
Here's a patch to modules/simpletest/tests/common.test, providing the test. I'm not completely sure if this is the way it's supposed to be. If I've missed anything, please let me know.
#3
Patch apply's and works but there were coding standard violations. Spaces around concatenation, spaces in front of comments and caps at beginning of comments.
#4
Whoops forgot to change back
#5
I've reviewed this, it's good.
#6
Since this is a testing party patch, I'm going to be very nit-picky in this review. It's only because I care. :)
<?php+class UrlTestCase extends DrupalWebtestCase {
?>
Let's add a PHPdoc block with a quick sentence that summarizes what this test case does.
<?php+ 'description' => t("Performs tests on the url() function"),
?>
a) Should be single quotes, not double, since there's no interpolation going on.
b) Should end in a period.
<?php+ 'group' => t('System')
?>
Should be a comma at the end of this line. I know it looks like a mistake, but it's the standards. :)
<?php+ /*
+ * Test case
+ * Assert that calling url() with an external URL
+ * 1. containing a fragment works with and without a fragment in $options.
+ * 2. containing or not containing a query works with a query in $options.
+ */
?>
a) /* needs to be /**
b) Test case isn't very descriptive. I would replace that with a one-sentence summary, like "Test the url() function's $options array." Make sure there's a newline between this sentence and the lines below it with more detail.
<?php+ $this->assertEqual($test_url . '&' . $query2, $result_url, t('External URL with a query'));
...
+ $this->assertEqual($test_url . '?' . $query2, $result_url, t('External URL without a query.'));
?>
The last messages were nice and descriptive as to what was being tested. These two, a little less so...
<?php+ $test_url = 'http://www.drupal.org';
?>
That's weird that the code below passes. Looks like this needs a trailing /.
That's it. :) Thanks a lot!!
#7
Hopefully this one is better.
url('http://drupal.org', array('query' => 'param1');
returns http://drupal.org?param1
So the ending slash is not needed.
#8
Added the test function to the other descriptions.
#9
The last submitted patch failed testing.
#10
#11
getInfo() should no longer have PHPdoc, see http://drupal.org/node/325974
Also it looks like the calls to t() could use double quotes to save escaping the single quotes.
#12
Reroll according to #11.
#13
#14
@lilou: link to patch in #12 doesn't seem to be working (directs me to the d.o. home page), and the testbot isn't picking this one up for testing. Could you re-upload?
#15
#16
The last submitted patch failed testing.
#17
Now tests may passes.
#18
The last submitted patch failed testing.
#19
Moving to the appropriate component since url() is base of the base system.
#20
tests look good, all pass. rerolled against HEAD.
#21
Attached patch remove
t()ingetinfo()(#500866: Simpletest: remove t() from assert message, test getInfo(), etc).#22
The last submitted patch failed testing.
#23
Patch applied without fuzz and tests pass. Not sure why testbot is complaining of a warning in the Javascript test.
As a side note though, we now have 3 tests in the System group that all deal with the url() function:
#24
Whoops. Sorry this took so long to get back on my radar. :\
Committed to HEAD, thanks!
#25
Automatically closed -- issue fixed for 2 weeks with no activity.