Improve test coverage for ExtensionDiscovery

Calling the new test ExtensionDiscoveryKernelTest, so we can distinguish it in conversations more easily.

Undecided between static::assertEquals() and $this->assertEquals().
In the current version of PhpUnit, it is clearly a static method. And my IDE will complain if I use $this->.
But in the previous issue, my static:: was changed to $this->.

Please don't just silently "fix" this, but explain why we use $this->assertEquals() in Drupal core.

Another note:
The two helper methods in ExtensionDiscoveryTest are protected instead of private or public. This suggests that they are meant for reuse, but the only way to reuse them is inheritance. I disagree with this. If we want reuse, they should rather be public static and live in a separate class. If we don't want reuse, they should be private.
But so it was done, and so shall it remain.

Any reason this is static?

No. This is a bug.

Extreme frowny face.

Yeah. But we are testing existing behavior. So I think it is ok for this issue, and we can change it later.

Let's assert that it's not found so that we'll come back and change this if we need to.

This is already being asserted with the assertEquals() on arrays, which asserts that the array has exactly those values and keys that are expected, not more and not less. The test will fail if you change the sitePath handling in ExtensionDiscovery.
Imo this is fine as it is.

Re: private and protected, the real problem in ExtensionDiscoveryTest is that there's a function that should be a data provider or at least just a helper method, but instead it builds *all* the dependencies for you, magically. That's why testVfs() is the wrong way to go: The test should be refactored so that dependencies and expectations are set up in the test method, so you can read them. Testing the test is a bad pattern.

Data providers are useful if you want to run the same test more than once, with different parameter values.
Otherwise, I don't see an advantage compared to a helper method directly called from within the test.

The expectations in the different test methods are very similar, with a few changes only. Therefore, it makes sense to do the main part of the expectations setup in a shared helper method, instead of the test itself.

The helper could be changed to be more verbose and more explicit. Maybe this would improve readability, maybe not. I personally doubt it.
The price for the dynamic setup is that, if a test fails, we have to consider that our dynamic setup went wrong.
Therefore there are test methods to check whether the dynamic setup works as expected.
It is certainly unconventional, but I don't see any real problem with it.
The alternative, as said, is a more verbose and explicit setup. I personally don't see this as the preferable option.

Anyway, for the purposes of Drupal, there's no such thing as private because @dawehner doesn't like it. :-) So protected is what we have.

I am not happy, as I explain here: #2727011: [policy, no patch] Private vs protected, and the role of inheritance
But I am ok to move forward with protected, until significant people change their minds.

We use them as $this-> but they're static to support non-OOP use:

Ok.
It is just sad for my IDE. I like my files to appear with a green checkbox.
Now I either have to live with these IDE notices, or I have to disable this inspection. Not really happy with either.
But if this is how we do things, then ok let's keep it this way.

Waiting for more opinions / review(s). Then I will post another patch.

Can't you setup a second vfs root?

This would make things easier indeed.
I had some difficulties wrapping my head around the API of vfsStream.
If you can post a simple example how to achieve this, I would be grateful.
Also, if you know a way to register slash-separated paths instead of nested array structures.

Would it be possible to move this bit to a second test function?

Why would we? These assertions are all on data from the same operation being tested.
Putting this into a separate method would mean the same operation, with same input data, would be run twice.

There we have it.

    vfsStream::setup('example1', null, ['dir1' => ['file1' => '123']]);
    scandir('vfs://example1');  // Works.
    vfsStream::setup('example2', null, ['dir2' => ['file2' => '123']]);
    scandir('vfs://example1');  // Fails.

The first scandir() works, the second one fails.
Fail at the second scandir():

scandir(vfs://example1): failed to open dir: "org\bovigo\vfs\vfsStreamWrapper::dir_opendir" call failed

It seems that vfsStream can only handle one root directory at a time, globally.

Btw, if I use [] instead of null as the second argument, even the first one fails, I suppose for permission reasons.

But I find now that the following code works, by reusing the same root directory:

    vfsStream::setup('example1', null, ['dir1' => ['file1' => '123']]);

    static::assertEquals(
      [
        '.',
        '..',
        'dir1',
      ],
      scandir('vfs://example1'));

    vfsStream::create(['dir2' => ['file2' => '123']], vfsStreamWrapper::getRoot());

    static::assertEquals(
      [
        '.',
        '..',
        'dir1',
        'dir2',
      ],
      scandir('vfs://example1'));

I see what I can do with this :)

Now everything with vfsStream :)
Who said it cannot be done?

EDIT: Oh no I did it again. Unintended modifications in ExtensionDiscovery. Ignore this patch.

Trying again.

Is there anything else we need to test, with or without kernel?

@Mile23 Thanks for the review.

Tests should not assert the validity of their fixtures. Declaring the fixture itself is defining the expectation. That's why it should be easy to read. If the fixture code is so complex that you have to test it, you're headed down the wrong path.

Okok you are right.

I was shying away from the big verbose explicit array definition. But ok, it is the right thing to do.

This code will be run for every test, and will set up expectations and cause assertions before the test even starts.

That means all tests which rely on this class are risky.

It also means that you are setting expectations for all future tests which will be added to this class. That will likely be a problem later on.

The vfsStream setup is shared between test methods. If I do this setup in one method, then the next method will see the same directories in vfsStream, unless it overwrites them. By doing it in the setUp() method, at least we are honest.

This leaves the following options:
- Let each test method clean up after itself.
- Tell PhpUnit to run each test method in its own separate process (which I think is possible with a doc tag or something).
- Let each test method that uses vfsStream override any existing setup.
- Let each test method use a different virtual subdirectory.
- Do a shared filesystem setup in the setUp() method. This is what I chose to do in the patch.

If we make the expectations setup explicit, and decouple it from the filesystem setup, then only the filesystem setup is left in the setUp() method.

I don't think leaving this there is as big a problem as you make it out to be.
Future test methods that do not wish to use this specific directory structure can register additional directories elsewhere.
E.g. vfs://root vs vfs://root2

Next patch will follow when we make a decision about the above options.

Tests should not assert the validity of their fixtures. Declaring the fixture itself is defining the expectation. That's why it should be easy to read. If the fixture code is so complex that you have to test it, you're headed down the wrong path.

If we have a good reason why the fixtures code should be complex, or if the fixtures depend on a fragile or intransparent tool, then I see nothing wrong with testing the fixtures first.

This premise needs to be questioned, of course. Is the complexity really necessary or helpful?
In our case, the dynamic setup of expectations was not necessary. So here it is better to use an explicit array definition and get rid of the assertions.

For an outsider, vfsStream has to appear like a fragile and intransparent tool. Is some global PHP configuration is messing with vfsStream? Maybe we are not using vfsStream correctly?
Seeing a few assertions that verify that vfsStream works correctly can be reassuring for a reader.

And seeing such an assertion fail is certainly preferable to a failure further down, where we are not sure if vfsStream is wrong, or the component we are testing is wrong.

This patch gets rid of the ridiculously dynamic building of the expectations array.
Also the root path is now hardcoded, so we get rid of all object variables.

The vfs setup is still shared between all test methods.
I think this is kind of ok.