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.

Files: 
CommentFileSizeAuthor
#13 2099679-13-rest-assert-order.patch1.41 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 58,844 pass(es).
[ View ]
#11 interdiff.txt2.64 KBlinclark
#10 2099679-10-rest-assert-order.patch4.08 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 58,820 pass(es).
[ View ]
#8 2099679-04-rest-assert-order.patch3.57 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 59,125 pass(es).
[ View ]
#5 2099679-04-rest-assert-order.patch0 byteslinclark
PASSED: [[SimpleTest]]: [MySQL] 59,006 pass(es).
[ View ]
#5 interdiff.txt1.27 KBlinclark
#3 2099679-03-rest-assert-order.patch3.36 KBlinclark
PASSED: [[SimpleTest]]: [MySQL] 59,209 pass(es).
[ View ]
#1 2099679-01-rest-assert-order.patch1.42 KBlinclark
FAILED: [[SimpleTest]]: [MySQL] 59,042 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.42 KB
FAILED: [[SimpleTest]]: [MySQL] 59,042 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Just a patch to demonstrate the failure.

Status:Needs review» Needs work

The last submitted patch, 2099679-01-rest-assert-order.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.36 KB
PASSED: [[SimpleTest]]: [MySQL] 59,209 pass(es).
[ View ]

This patch breaks the test into its two component parts.

Status:Needs review» Reviewed & tested by the community

Nice catch! Test pass locally and patch looks OK. Let's wait for the testbot to re-confirm.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.27 KB
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,006 pass(es).
[ View ]

Just some comment fixes.

Sorry, crosspost. If passes testbots, I'll just re-RTBC it.

Status:Needs review» Needs work

Empty patch ;D

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new3.57 KB
PASSED: [[SimpleTest]]: [MySQL] 59,125 pass(es).
[ View ]

Would help if the patch actually contained something...

moving back to jaumpy's RTBC while I'm at it.

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/rest/lib/Drupal/rest/Tests/AuthTest.php
    @@ -44,9 +57,10 @@ public function testRead() {
         // Try to read the resource as an anonymous user, which should not work.
    -    $response = $this->httpRequest('entity/' . $entity_type . '/' . $entity->id(), 'GET', NULL, $this->defaultMimeType);
    +    $this->httpRequest('entity/' . $entity_type . '/' . $entity->id(), 'GET', NULL, $this->defaultMimeType);
         $this->assertResponse('401', 'HTTP response code is 401 when the request is not authenticated and the user is anonymous.');
         $this->assertText('A fatal error occurred: No authentication credentials provided.');

    This should not be asserted in a setUp() method.

  2. +++ b/core/modules/rest/lib/Drupal/rest/Tests/AuthTest.php
    @@ -55,18 +69,32 @@ public function testRead() {
    +  /**
    ...
    +   */
    +  public function testDisabledAuth() {

    Comment should be improved: "Tests that the disabled cookie auth results in 401 response."

  3. +++ b/core/modules/rest/lib/Drupal/rest/Tests/AuthTest.php
    @@ -55,18 +69,32 @@ public function testRead() {
    +    $this->curlClose();

    Are you sure that we need this? Each test method is an isolated test, so this is not necessary.

  4. +++ b/core/modules/rest/lib/Drupal/rest/Tests/AuthTest.php
    @@ -55,18 +69,32 @@ public function testRead() {
    +  /**
    +   * Test that enabled auth results in a 200 response.
    +   */
    +  public function testEnabledAuth() {
    +    // Try to read the resource with Basic authentication, which is enabled and
    +    // should work.
    +    $this->basicAuthGet('entity/' . $this->entity->entityType() . '/' . $this->entity->id(), $this->account->getUsername(), $this->account->pass_raw);
    +    $this->assertResponse('200', 'HTTP response code is 200 for successfully authorized requests.');
    +
         $this->curlClose();
       }

    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.

Status:Needs work» Needs review
StatusFileSize
new4.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,820 pass(es).
[ View ]

This should not be asserted in a setUp() method... And the anonymous request without any auth should be moved here + the the test method comment should be updated.

Agreed 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.

Are you sure that we need this? Each test method is an isolated test, so this is not necessary.

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.

StatusFileSize
new2.64 KB

Forgot the interdiff

I'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?

StatusFileSize
new1.41 KB
PASSED: [[SimpleTest]]: [MySQL] 58,844 pass(es).
[ View ]

That 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.

Category:task» bug
Priority:Normal» Major

Upping to major since we can't be sure of test results until this is fixed.

Status:Needs review» Reviewed & tested by the community

Looks good to me.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Updated issue summary.