Avoid Request::factory() calls in test providers

[enov]

Hello all,

It is funny, sad and frustrating at the same time to know why Kohana is not marking 100% in the official HHVM OSS frameworks tests suite.

Background

I requested a pull to update the tested Kohana framework version in HHVM framework tests to v3.3.4. It was surprising for me to see it passing only at 99.85%. HTTPTest::test_redirect was the failing test. However, when running PHPUnit tests from within the Kohana folder, the standard way, all was OK and tests were passing.

I had hard time to figure out the real cause. Took me around 3 days, with a cumulative 12 hours. Even when I run HTTPTest alone with the --filter=HTTPTest flag from within the standard Kohana installation folder, the test always passed. I tried in vain to find ways to debug this. At some point, I was furious enough to delete all other test files from the disk except HTTPTest.php, in order to "force" PHPUnit to run with only HTTPTest.php phisically available. It was only then, to my surprise, that the test failed. A clear indication that some tests are affecting others. I hunted down and found out the culprit. It was the RequestTest that was affecting HTTPTest.

Cause

Request::factory() calls in test "provider" methods initializes the Request::$initial static variable, affecting other test methods, even if those "provider" methods are not the direct providers of those other test methods.

In our case the HTTPTest::test_redirect is affected because the HTTP::redirect method needs the Request::$initial static variable to be available. Here's the pseudo call stack:

---

HTTP::redirect() ▶️ HTTP_Exception_Redirect::location() ▶️ URL::site() ▶️ URL::base() which uses Request::$initial

---

This becomes problematic because HTTPTest::test_redirect is now dependant of the RequestTest "providers", as those RequestTest "providers" change the global state in favor of HTTPTest's needs. Once RequestTest "providers" are not available -- ex: by deleting RequestTest.php file from disk -- HTTPTest::test_redirect test cases fail.

Solution (well... for the moment)

This is a reported issue and we lengthily discussed this elsewhere already... For the moment, however, this is what I am proposing for v3.3:

In order to not alter the global state, and to not affect other tests: static variables initialization, and calls to methods that affect static variables should not be made in test case providers. Rather, these calls should be done carefully in setUp and tearDown methods, as well as within the test methods themselves.

In this case, I have removed Request::factory() calls from providers and restructured the tests.

Note that:

• a bug was found and fixed at 70b88e8 after restructuring the tests.
• without c37cf6f HTTP::redirect() redirects with protocol cli ex: cli://www.example.com/kohana/index.php/page_two
• In c37cf6f it is probably better use NULL instead of using $this->_initial_request. But preferred to keep things along the way it's done in RequestTest.

This is ready for merge, please review thoroughly, cc @acoulton.

Thanks!

[fredemmott]

reverting the hhvm diff, does not work:

ERROR: Could not get tests for kohana

[enov]

@acoulton any hope you could review and merge this :)

[acoulton]

@enov sorry, I was away on holiday last week. This looks good to me, thanks for fixing.

Perhaps we should also add something to the Unittest_Testcase setup that clears as many as possible of the statics (set them NULL for eg) to avoid issues like this creeping back in in future?

[enov]

Thanks @acoulton

Perhaps we should also add something to the Unittest_Testcase setup that clears as many as possible of the statics (set them NULL for eg) to avoid issues like this creeping back in in future?

That might be a good idea. But I am not sure how to do: For some reason, all test provider methods seem to be always running/available... So even if we setUp and tearDown things properly in the base Unittest_Testcase, I don't think there is any guarantee that we will be in a clean state when the runtime enters into the body of a test method.

[acoulton]

@enov ISTR PHPUnit does a first pass to load the list of all tests - which includes running all providers, because it can't tell how many tests there are till it knows how many sets of arguments each provider returns. Then it filters the list, and then it starts actually running tests - always calling setUp and tearDown around each test method execution.

So you can always guarantee setUp will fire (in a new class instance) just before each execution of a test method.

[fredemmott]

https://phpunit.de/manual/current/en/appendixes.annotations.html#appendixes.annotations.backupStaticAttributes may be sufficient.

If you want to find or assert when a tearDown() is leaving state, you could add a static function with @afterClass to your test base class that takes a look at ReflectionClass::getStaticProperties(), or an additional cleaner with @beforeClass

As an aside, the update to the HHVM test suite was un-reverted; this turned out to be an unrelated infrastructure issue that would have been triggered by any update. Sorry for the noise.

[enov]

@acoulton

always calling setUp and tearDown around each test method execution.

My preliminary attempts seemed to show that PHPUnit was NOT following that rule. The way providers of RequestTest were affecting HTTPTest, even after a tearDown, made things complicated. So I thought either there's a bug in Kohana's unittest module, or a maybe a bug in PHPUnit.

However, now that I am still preparing a reply to your comment, I noticed that in setUp of RequestTest, we are backing up the already tainted Request::$initial into a class property $this->_initial_request = Request::$initial;, only later to restore it in tearDown. It is pretty confusing.

@fredemmott

Thank you for your hard work. I'll dig deeper into those, or see if a base setUp will do the job.

Cheers!

[fredemmott]

The advantage of @beforeClass is that it should still be executed if a subclass overrides setUp and doesn't call parent.

[hkdobrev]

I think 70b88e8 causes a bug when internal sub requests are made.

So if there's an initial request already and you do an internal sub request with:

Request::factory('/example?foo=bar')->execute()

and then in the action matching the URI of this sub request you try to get $this->request->query() it will have different behaviour:

In Kohana 3.3.4 and before:

$this->request->query() = ['foo' => 'bar']

In Kohana 3.3.5:

$this->request->query() = []

I think this is a major bug introduced in Kohana 3.3.5.

It could be worked around using:

Request::factory('/example')->query(['foo' => 'bar'])->execute()

However setting query params in the URL of Request::factory() used to work and now is broken.

[enov]

@hkdobrev thanks for reporting. As far as I remember, some tests might fail if we revert 70b88e8. I still did not look deeply into your example though.

[hkdobrev]

Just to note I couldn't find any documentation describing the case of including query params in the URL string passed to Request::factory(). However there is no documentation or example prohibiting it and it used to work and that's why I consider it a bug or at least an unexpected and breaking change of behaviour in a patch version increase.