Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment #N
Problem/Motivation
If you switch the order of the requests and corresponding assertions in testRead, both of them fail.
Proposed resolution
Unset the curlHandle in between assertions to ensure that the settings don't carry over from request to request.
Comment | File | Size | Author |
---|---|---|---|
#13 | 2099679-13-rest-assert-order.patch | 1.41 KB | linclark |
#11 | interdiff.txt | 2.64 KB | linclark |
#10 | 2099679-10-rest-assert-order.patch | 4.08 KB | linclark |
#8 | 2099679-04-rest-assert-order.patch | 3.57 KB | linclark |
#5 | 2099679-04-rest-assert-order.patch | 0 bytes | linclark |
Comments
Comment #1
linclark CreditAttribution: linclark commentedJust a patch to demonstrate the failure.
Comment #3
linclark CreditAttribution: linclark commentedThis patch breaks the test into its two component parts.
Comment #4
juampynr CreditAttribution: juampynr commentedNice catch! Test pass locally and patch looks OK. Let's wait for the testbot to re-confirm.
Comment #5
linclark CreditAttribution: linclark commentedJust some comment fixes.
Comment #6
linclark CreditAttribution: linclark commentedSorry, crosspost. If passes testbots, I'll just re-RTBC it.
Comment #7
juampynr CreditAttribution: juampynr commentedEmpty patch ;D
Comment #8
linclark CreditAttribution: linclark commentedWould help if the patch actually contained something...
moving back to jaumpy's RTBC while I'm at it.
Comment #9
klausiThis should not be asserted in a setUp() method.
Comment should be improved: "Tests that the disabled cookie auth results in 401 response."
Are you sure that we need this? Each test method is an isolated test, so this is not necessary.
Same for the curlClose() here, can be removed. And the anonymous request without any auth should be moved here + the the test method comment should be updated.
Comment #10
linclark CreditAttribution: linclark commentedAgreed on the first point. However, I disagree on the second. The tests were order-dependent before because of unintended interactions between calls to httpRequest. If we have two calls to httpRequest in testEnabledAuth, then aren't we opening ourselves up to the same situation? I guess one could make an argument that there is no credential being sent so no headers being set...
For now, I've made it its own test.
No, I don't think we do need this. I put it in there because it was included at the end of the test in the original, but it seems that PHP closes out curl requests when the variable goes out of scope at the end of the function, so no need. I have removed it.
I improved the comments in line with what was suggested as well.
Comment #11
linclark CreditAttribution: linclark commentedForgot the interdiff
Comment #12
klausiI'm not really happy with 3 test methods since that makes running the tests slow and tedious. The time goes up from 30 seconds to 1:30 on my laptop. Installing Drupal 3 times just to test this seems wrong to me.
We could also use unset($this->curlHandle) after each request to just clean out any annoying cURL options that we cannot get rid off. That way the cURL browser is newly initialized every time we make a request. What do you think about that?
Comment #13
linclark CreditAttribution: linclark commentedThat does seem to work. The problematic combination is when No Auth comes directly before Basic Auth. When they are ordered in this way, the test fails, but when you unset the curlHandle between the two, it works.
Comment #14
linclark CreditAttribution: linclark commentedUpping to major since we can't be sure of test results until this is fixed.
Comment #15
klausiLooks good to me.
Comment #16
catchCommitted/pushed to 8.x, thanks!
Comment #17.0
(not verified) CreditAttribution: commentedUpdated issue summary.