TestingParty08: drupal_http_request redirects need a test
chx - August 17, 2008 - 08:10
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | tests |
| Category: | bug report |
| Priority: | critical |
| Assigned: | domas |
| Status: | closed |
Description
Assert that if you drupal_http_request a page and that issues a 301, 302, 307 response code then it's redirected properly if and only if when the appropriate parameter is passed to drupal_http_request /
#1
#296307: TestingParty08: drupal_http_request basic auth needs a test already sets up a dummy module for drupal_http_request. That patch can probably be used as a starting point for the current issue.
#2
This is the patch adding the tests (I started working during the testing party, but didn't finish it on time). I have included the above mentioned patch.
If there are any comments regarding the code quality (or anything else for that matter), please let me know.
#3
Few very minor things:
Implementation of setUp().to added setUp() method.Test looks good, ran test and it passes.
#4
This patch includes the test by flobruit and nickske from: #296307: TestingParty08: drupal_http_request basic auth needs a test which I have marked as a duplicate.
#5
the .module should be:
<?php
// $Id$
/**
* Implementation of hook_menu().
*/
function drupal_http_request_test_menu() {
$items = array();
$items['drupal_http_request_test'] = array(
'page callback' => 'drupal_http_request_test_basic_auth_page',
'access callback' => TRUE,
'type' => MENU_CALLBACK,
);
$items['drupal_http_request_test/%'] = array(
'title' => t('Redirect'),
'page callback' => 'drupal_http_request_test',
'access arguments' => array('access content'),
'type' => MENU_CALLBACK,
);
return $items;
}
function drupal_http_request_test_basic_auth_page() {
$output = t('$_SERVER[\'PHP_AUTH_USER\'] is @username.', array('@username' => $_SERVER['PHP_AUTH_USER']));
$output .= t('$_SERVER[\'PHP_AUTH_PW\'] is @password.', array('@password' => $_SERVER['PHP_AUTH_PW']));
return $output;
}
function drupal_http_request_test($code) {
if ($code != 200) {
header("Location: " . url('drupal_http_request_test/200', array('absolute' => TRUE)), TRUE, $code);
exit;
}
return '';
}
?>
#6
Sure, that is cleaner. Missing 'page arguments', but otherwise it works.
#7
* I'm not sure we should create a dedicated module for this test. It feels like it would be better to create a more generic system_test.module or something along these lines.
* We don't use underscores in URLs. Use dashes instead, and consider to shorten the URLs.
#8
I'm on it - will fix this soon.
#9
Ok, I've created a system_test module, that does all the support work. The patch includes the changes made in #296299: TestingParty08: drupal_get_destination with a query needs a test and #296307: TestingParty08: drupal_http_request basic auth needs a test.
Looking forward to any replies regarding the code quality. (or anything else for that matter)
#10
Extended tests with parameter "$retry" set to "0".
Added redirect to invalid URL tests.
#11
Tests look thorough, pass, and now use a standard system_test.module.
Looks like good work.
Test results:
19 passes, 0 fails, 0 exceptions#12
Excellent. Thanks for incorporating my feedback so quickly. Committed to CVS HEAD. Thanks ...
#13
I get unresponsive script errors whenever I run this test in FF3 on OSX. Didn't get a chance to look more closely at it yet.
#14
I looked in the simpletest table in the database. This is the last row to get inserted before it times out:
"The query passed to the page is correctly represented by drupal_get_detination()."
It's very strange, because that's the last assertion in common.test. I tried commenting its parent function, testDrupalGetDestination(), out but no dice.
#15
Really strange. The tests work fine for me (FF3 under linux). Can anybody else reproduce this problem? Coudl it be not due to the test itself? Do any other tests cause this?
#16
Runs fine FF3 SUSE 11.0 (linux).
#17
#18
I'll try digging in and debugging this tonight. Good to know Linux is unaffected. I'll see if I can scrounge up some Mac users to test.
#19
http://drupal.org/node/250047#comment-996555
#20
Oh, god, that test was *silly*.
<?php$this->assertEqual($redirect_301->redirect_code, 200, t('drupal_http_request follows the 301 redirect.'));
?>
Come on! That's a "if there is a bug, print that there is none".
@webchick: what you were experiencing are notices in drupal_http_request that broke the batch API somehow.
This patch fixes both drupal_http_request and the test.
#21
Committed. Love it when tests find bugs. Yay! :)
#22
Sorry, my bad. For some reason I thought redirect_code is supposed to return the code of the final response, which should be 200 (response of the page we redirected to).
#23
Sweet!
#24
This fix was wrong and should be reverted. The real fix to the bug is here: http://drupal.org/node/293529#comment-960905
#25
Let's this issue die peacefully. I'll answer in the other one.
#26
Automatically closed -- issue fixed for two weeks with no activity.