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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

linclark’s picture

Status: Active » Needs review
FileSize
1.42 KB

Just a patch to demonstrate the failure.

Status: Needs review » Needs work

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

linclark’s picture

Status: Needs work » Needs review
FileSize
3.36 KB

This patch breaks the test into its two component parts.

juampynr’s picture

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.

linclark’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.27 KB
0 bytes

Just some comment fixes.

linclark’s picture

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

juampynr’s picture

Status: Needs review » Needs work

Empty patch ;D

linclark’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.57 KB

Would help if the patch actually contained something...

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

klausi’s picture

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.

linclark’s picture

Status: Needs work » Needs review
FileSize
4.08 KB

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.

linclark’s picture

FileSize
2.64 KB

Forgot the interdiff

klausi’s picture

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?

linclark’s picture

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.

linclark’s picture

Category: task » bug
Priority: Normal » Major

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

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

catch’s picture

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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.