-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Data Providers get executed before setupBeforeClass #836
Comments
I'm not sure that enforcing some order so that static resources can be initialized in Why not just lazy initialize the adapters? Then the order doesn't matter at all. |
Without running the data provider we do not know how many tests a test method runs. In a perfect world we would break B/C and require data providers to be objects that implement an interface that extends |
@whatthejeff This is what we end up doing. However initializing in the constructor could be a little bit cleaner, because parameters have to be wrapped in an array when returned from a provider. @sebastianbergmann ok I understand why they are called before now, what about mentionning it in the doc ? (I can open a PR which the changes if you are ok). I am not even sure having objects would help a lot: if you look at my ex, there is a dependency from one provider to the other. Thanks for your replies, closing the issue. |
@vicb, I'll gladly merge a documentation-related PR for this. |
I might be a bit late to the scene, but what I happen to do when I have dependencies that I need to initialize is to ditch the |
@srosato I'm being dumb (I've not used yield). Could you give a brief example of how to do this please? Via https://gist.github.com if an example here is not appropriate. |
If your public function myProvider() {
return [
'new Klass("param 1", "param 2")',
'new Klass("param 1", "param 2")',
];
}
/**
* @dataProvider myProvider
*/
public testMyFunction($instance_str) {
$klass = eval("return {$instance_str};");
# continue testing $klass ...
} |
my two cents: Instead of a dataprovider relying on a static property (which we know isn't possible) I am setting the needed data/objects as class member(s) of the test class in as long as there aren't too many test-class-wide dependencies, i feel comfortable with this approach. specifically, i use a DB connection as static property which is set in |
A solution would be defining a new private method and define as many variables as needed, as See it in an example: private static function getData()
{
static $data, $anotherData;
if (!isset($data)) {
$data = new TestClass();
$anotherData = [];
}
return [
$data,
// Or: clone $data
$anotherData,
];
}
public static function setUpBeforeClass()
{
list(self::$sampleJson, self::$sampleData) = self::getData();
}
public function sampleProvider()
{
$data = self::getData();
return [
$data
];
} |
@machitgarha thanks to your comment yesterday I found out about this ticket. :) I'm working on refactoring the data provider logic in #3736 which will solve some of the common issues:
|
@epdenouden Happy hearing that! ;) The second item in your list is a good one. Waiting for that.
Isn't it currently possible? I always use non-static data providers in my tests instead of static ones, and they work as expected with no issues (using PHPUnit 7.5.6). Am I wrong? |
No, you are correct! My phrasing wasn't precise enough, thanks for bringing this up. Deep down in the data provider handling code it currently reads: private static function getDataFromDataProviderAnnotation($allTheParameters): ?iterable
// code for locating the data provider
// [...]
if ($dataProviderMethod->isStatic()) {
$object = null;
} else {
$object = $dataProviderClass->newInstance();
}
// code for preparing returned data rows
// [...]
} So here's the dirty little secret:
You gave me something to think over and validate regarding the data provider refactoring! I need to add extra tests to make sure the object instances are the expected ones, not just the same type or a clone. ☕️ and 🍰 on me when you're in Amsterdam. |
@machitgarha The instances are indeed different: https://github.com/epdenouden/phpunit/blob/4718e0bb470170b2da46bf0e05b7275110a2a29d/tests/_files/DataProviderInstanceTest.php#L14-L27 |
@epdenouden But I don't get any failures! In PHPUnit 7.5.13, the assertion doesn't generate any errors. How do you get this error? Backward-incompatibility, you mean? |
This is in the lazy loading data provider branch I am working on which is based on 8.2. I will check 7.5.x next, thanks for the heads-up. If this works I'll have to go back to some other older issues that were asking explicitly to implementing non-static provider methods and look at the exact use cases again. In any case: your comment has already inspired a very useful test :) |
And @machitgarha yes that would be a BC-break |
@epdenouden I'll wait for that. For some reasons, in my project, I'm using PHPUnit 7.5.13; but I'm going to update it to 8.2.* soon. And good news, if the BC-break could be fixed. I don't know the structure of the PHPUnit itself, so I don't know what you changed, but I guess that it is possible to fix it. However, it's all up to you! :) |
Introducing a backward compatibility break without a very good reason is not something that @sebastianbergmann would allow, so it will be fixed before going into the next version. PHPUnit is a tool used to guard software quality in the PHP community and not my personal playground. So yes, please tell us what you would like to see as an end-user/developer of tests. Just keep in mind projects like these do not have a large pool of volunteer developers. |
Fun fact: the test above indeed works on both versions 7.x and 8.x and fails on the JIT prototype. The code I am working on is being much more like the the original code flow still. So it might be the result of a bug, some still logic I still need to refactor further, something low-level about objects and reflection in PHP I need to look up or maybe just some naive testing on my part. 🔬 |
@epdenouden JIT? Are you testing with JIT for PHP, or there's something I don't know? Sorry, I'm a bit confused; are you talking about PHP7.x and PHP8.x or PHPUnit7.x and PHPUnit8.x?! |
@machitgarha I understand your confusion as PHP8 is getting a just-in-time compiler. I was using the term for the new data provider implementation which is based on the same idea: a just-in-time loader a.k.a. lazy-loading. Initialize only what you need, when you need it. This will work fine with the upcoming PHP version 8 JIT as it doesn't use any tricky features of the PHP language itself. All my work is deep inside PHPUnit redoing things like configuration parsing, test loading and running, etc. If it looks like I am not doing anything, I am doing it right. Except for that lowered GCP or AWS invoice. 💸 |
The data providers get called before the static
setupBeforeClass
has been executed. If think it should be the other way around.Use case:
setupBeforeClass
and have the provider returns that propertyThe problem is that the property is not initialized when the provider gets called.
Version found: 3.7.14
The text was updated successfully, but these errors were encountered: