Cookies from previous tests are still present when a new test starts

1. +++ b/core/modules/simpletest/src/Tests/BrowserTest.php
   @@ -65,4 +65,34 @@ function testXPathEscaping() {
   +    // Check that the name and value of the cookie match the request data.
   +    $cookie_headers = preg_grep('/^Set-Cookie: ([^=]+)=(.+)/', $this->headers);
   +    $cookie_header = reset($cookie_headers);
   

   Use $this->drupalGetHeader() unless there is a strong reason against that.

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1089,6 +1089,7 @@ protected function tearDown() {
        $this->curlCookies = array();
   +    $this->cookies = array();
   

   The $cookies property is not initialized on declaration. In order to maintain consistency (each test should work with the same initial value), I think either the $cookies property needs to be set to NULL here or it should to be initialized to an empty array on declaration.

Thanks!

+++ b/core/modules/simpletest/src/Tests/BrowserTest.php
@@ -65,4 +65,40 @@ function testXPathEscaping() {
+  public function testCookies() {
...
+   * In order for this test to be effective it should always run after the
+   * testCookies() test.
...
+  public function testCookieDoesNotBleed() {

Let's set a static in testCookies() and assert it's value in testCookieDoesNotBleed() so we can be sure we're testing something - or if someone has the bright idea of parallelising all the individual test methods during a run this breaks.

Core maintainers comments addressed and reversed test sequence fails as desired. I would have set this to RTBC, but:

+++ b/core/modules/simpletest/src/Tests/BrowserTest.php
@@ -65,4 +73,44 @@ function testXPathEscaping() {
+    $this->assertTrue(self::$cookieSet, 'A cookie has been set in a previous test.');

'A cookie has been set in a previous test.' does not exactly tell why the assertion failed. A more explicit message like 'testCookieDoesNotBleed() runs after testCookie()' or so would be more helpful, especially in the case of parallel testing.

+++ b/core/modules/simpletest/src/Tests/BrowserTest.php
@@ -65,4 +73,44 @@ function testXPathEscaping() {
+    self::$cookieSet = TRUE;
...
+   * @see self::testCookies()
...
+    $this->assertTrue(self::$cookieSet, 'A cookie has been set in a previous test.');

I'm not quite sure and I cannot find any reliable source, but I feel that static is preferable over self nowadays. search query.

Re #12 perhaps something like Test methods are run in the expected order?

Thanks!

Committed ca5fced and pushed to 8.0.x. Thanks!

I am guessing this needs to be backported, right?

I have backported this issue to Drupal 7. Here is a testonly patch, that will fail.

Here is the patch itself. Please review.

Let me upload the testonly patch again, so it is picked up by the test-server.

As expected, the test-only patch failed. #24 passed, so I'm changing the status to Needs review again.

The test only patch failed again. #24, the patch with the fix is the patch up for review.

Nice, thanks.

+    // Set a flag indicating that a cookie has been set in this test.
+    // @see testCookieDoesNotBleed()
+    static::$cookieSet = TRUE;
....
+    $this->assertTrue(static::$cookieSet, 'Tests have been executed in the expected order.');

This won't work on PHP 5.2, which we technically still support. Is there any reason self:: can't just be used instead?

Also, minor, but the @see should actually be a sentence using "See" since it's an inline code comment. I would probably include the class name too, in other words: "See SimpleTestBrowserTestCase::testCookieDoesNotBleed()."

Oh, do we even have 5.2 test runners on CI?

I have changed the patch as suggested in #33. Here are a new patch and the interdiff.

Please review!

The test that failed has nothing to do with the patch, so marking Needs review again.

Looking better, but:

+    // @see See testCookieDoesNotBleed()

The @see should not be there. Just the "See". And a period at the end of the sentence.

+   * @see See self::testCookies()
+   */
+  public function testCookieDoesNotBleed() {

Since this one is part of the function documentation it should use @see but according to https://www.drupal.org/coding-standards/docs#see. So it should just be "@see SimpleTestBrowserTestCase::testCookies()"

+    $this->assertTrue(static::$cookieSet, 'Tests have been executed in the expected order.');

This one will need to use "self::" also.

Oh, do we even have 5.2 test runners on CI?

Not currently, but I've heard rumors they could exist in the future :) Bottom line, it's worth keeping the tests compatible with PHP 5.2 (like the rest of Drupal) if we can.

Fixed the comments made in #39. Please review.

Looks good, thanks.

+++ b/modules/simpletest/simpletest.test
@@ -380,6 +388,46 @@ EOF;
+   * @see self::testCookies().

This should still say SimpleTestBrowserTestCase::testCookies.

Could be fixed on commit, though.

+1, looks great

Still RTBC and marked for commit.

Committed and pushed to 7.x! Thanks!

Added to CHANGELOG.txt so that module authors know that their tests could have a side effect fixed.