Improve GeneralTest

[uuf6429]

• replace duplicate tests with data providers

Depends on #81.

[stof]

Please don't use data providers for those. In this testsuite, we aim for granular tests to make it easier to skip some of them in driver repositories when needed (older tests of the testsuite are much less granular than recent ones as we learned about that but we did no rewrite everything). And when we use data providers, we have a single test executed many times, so the skipping in driver repositories is still all or nothing.

[uuf6429]

@stof If I understand the problem about granularity, I think the problem you had in the past was with tests testing multiple things (in fact some tests are still like that). That's a problem of the test itself.

Regarding data providers, we can still be granular. Right now skipMessage is basically missing a 3rd argument, the provided data, which can be easily done:

    protected function checkSkippedTest()
    {
        $config = self::getConfig();
        $message = $config->skipMessage(get_class($this), $this->getName(false), $this->getProvidedData());
        if (null !== $message) {
            $this->markTestSkipped($message);
        }
    }

Let's keep it DRY, it doesn't make sense to repeat 8/10 lines in these tests just because of an extra parameter.

(in hindsight, making this change in a backward-compatible way might be challenging...one possibility is to add it as a new function to the config class and deprecate the old one)

[stof]

@uuf6429 but that would mean that any change to the provided data becomes a BC break as it might impact the detection in skipping logic. This sounds unmaintainable to me.

[uuf6429]

@stof let's take the changed code as an example (where the dataset contains both field and value).

If the downstream is filtering against a specific value, and we change that value, then yes it becomes a BC break.
If the downstream is filtering by field name and we change the value, then it should be fine.
Currently, if the test name also contains the field name or value, and we change the test name (because of the value or field name or whatever), then it also becomes a BC break.

I suppose what I'm saying is that a test with a good name would actually cause this problem as well.

I get what you're saying, but I don't think this change will make it really better or worse.

[uuf6429]

@stof Perhaps a good compromise is to use $this->getName(true) (test name + dataset name, if any), then at least it would be bound by the dataset name instead of the more arbitrary dataset contents.

[uuf6429]

@stof any consensus on this issue?

I find that excluding tests by dataset name caries the same risk of BC breaks as the test name.

On the other hand data providers are a well known and convenient feature, which actually improve maintainability by avoiding duplication.