drupal_http_request triggers error in addition to returning it & test cleanup
boombatower - August 14, 2008 - 23:44
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | tests |
| Category: | task |
| Priority: | critical |
| Assigned: | boombatower |
| Status: | closed |
Description
When running the Drupal HTTP request test an exception in generated. That can be removed by adding @ to parse_url().
Since the code checks to make sure that it parsed the URL successfully and returns error messages anyway it would seem to make sense that the function would mask the errors triggered.
Updated test to include actual page fetch test.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal_http_request.test.patch | 1.63 KB | Ignored | None | None |
#1
Is this a different approach/related to #283806: Notice appears when drupal_http_request called with an invalid URL? I found both when searching for information about the Drupal HTTP request test.
#2
I've commented on the other issue.
#3
Crtical since this is now our only remaining test with any kind of breakage, review forthcoming.
#4
Makes sense to me to rely on our internal error handling and suppress the php warnings in the function call rather than just the test. This removes the exception, adds an extra test, lovely.
#5
Please we can do better than that:
<?php+ $this->assertTrue(strpos($result->data, '<title>' . variable_get('site_name', 'Drupal') . '</title>') !== FALSE, t('Site title matches.'));
?>
#6
Meaning get rid of !== FALSE? The reason that is there is I'm not sure if assertTrue is type strict. If not that strpos returning 0 would fail even thought that is success.
Since the page should never have the as the first character we could consider that a fail anyway. I'll remove the !== FALSE.
#7
Updated, per #5-6.
#8
Still passes and already reviewed per #4.
#9
I should have been clearer. I meant that we have a whole facility for asserting things on pages with DrupalWebTestCase, we should use it.
#10
Oups, here is a new version that fixes an exception in DrupalWebTestCase::getUrl().
#11
I realized that, but didn't use it since you can't without setting the content. Wasn't sure that was something we wanted to do.
I like what you've done, I'll take a look at it shortly.
#12
I like that you can set the content now, it has grown on me after thinking about it. I have added a more detailed description of the uses for drupalSetContent() and fixed a formatting issue related to the function indentation.
The test still passes and we both have reviewed this format.
#13
Committed to CVS HEAD. Thanks.
#14
That means we are at 100% pass!
#15
YYYYAAAAAAAAAYYYYYYYYYY!!!!!!!!!
Best. Day. EVER! :)
#16
The tests committed with this patch were reverted by the DB:TNG patch. See: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/simpletest/tests/c...
#17
#18
Doesn't it just need to be reapplied...not worked on?
#19
Doesn't apply.
#20
Re-roll.
#21
Test still passes after applicate, as before...looks good.
#22
edit: wrong issue.
#23
#24
Code no longer applies.
#25
Patch at #20 still applies.
#26
Committed to CVS HEAD. Thanks.
temp
Automatically closed -- issue fixed for two weeks with no activity.
temp
Automatically closed -- issue fixed for two weeks with no activity.
temp
Automatically closed -- issue fixed for two weeks with no activity.
#27
Automatically closed -- issue fixed for two weeks with no activity.