I wonder how often it really needs to be mocked, versus just creating a temp file or using php://memory or php://temp.

There's also vfsStream if we need something more elaborate.

For that matter, testing anything in that method requires making assumptions about the contents of files in the filesystem and perhaps copy-pasting something about their contents. This doesn't seem like the best approach. The correctness of Language::getGrammarTransformations() shouldn't have to depend on the data that we happen to ship.

You could argue that the path should just be injected instead of hard-coded, of course. But then you still couldn't test, e.g., whether there was an attempt to read the file. For instance, I'm testing caching behavior now, and I can't directly test that the code doesn't try to read the file. All I can test is that it gets the value from the cache and doesn't set it, which is probably fine, but is not really what I want to know.

Injecting the directory to look for would work, I think. The test can then pass a fixture directory (from test/data, or tmp, or memory, or vfs) and assert it that it gets the correct outcome. There shouldn't be invisible caches between tests for things that it could vary by, so if it has a fresh instance, the only way it gets the right result is if it found the file in the fixture and processed it. I don't think we need to assert that its implementation detail of it having opened a file handle to that path and read the relevant bytes.

(Things like LocalisationCache::$testCache are an exception and indeed couldn't easily have a meta-level test test).

Since the point of a cache is to save disk reads, I think we would want to directly test that it doesn't do unnecessary disk risks. Otherwise the cache isn't doing its job. Just injecting the directory doesn't give you any way to directly test that the cache is saving disk reads, i.e., that it's behaving usefully.

You still wouldn't really need to inject anything there. You could refactor it to split the file-loading part into a separate protected method that takes the filename as a parameter, e.g.

/**
 * Get the grammar transformations data for the language.
 * Used like grammar forms, with {{GRAMMAR}} and cases,
 * but uses pairs of regexes and replacements instead of code.
 *
 * @return array[] Array of grammar transformations.
 * @throws MWException
 * @since 1.28
 */
public function getGrammarTransformations() {
    $languageCode = $this->getCode();

    if ( self::$grammarTransformations === null ) {
        self::$grammarTransformations = new MapCacheLRU( 10 );
    }

    if ( self::$grammarTransformations->has( $languageCode ) ) {
        return self::$grammarTransformations->get( $languageCode );
    }

    $grammarDataFile = __DIR__ . "/data/grammarTransformations/$languageCode.json";
    $data = $this->loadGrammarTransformations( $grammarDataFile );
    if ( $data !== false ) {
        if ( $data === null ) {
            throw new MWException( "Invalid grammar data for \"$languageCode\"." );
        }

        self::$grammarTransformations->set( $languageCode, $data );
    } else {
        $data = [];
    }

    return $data;
}

/**
 * Load a grammar transformations file.
 * This is separate from getGrammarTranslations() so it can be mocked for tests.
 *
 * @param string $grammarDataFile File name to load.
 * @return array[]|false|null False if the file is not readable, null if JSON decoding fails, otherwise the data.
 * @since 1.34
 */
protected function loadGrammarTransformations( $grammarDataFile ) {
    if ( !is_readable( $grammarDataFile ) ) {
        return false; // or whatever sentinel you want to use
    }

    return FormatJson::decode(
        file_get_contents( $grammarDataFile ),
        true
    );
}

Then you could test that loadGrammarTransformations() method by calling it using reflection (i.e. TestingAccessWrapper), and test getGrammarTransformations() by mocking loadGrammarTransformations() to return whatever data you want to test with.

It seems that your "whether there was an attempt to read the file" would be satisfied here too, by mocking loadGrammarTransformations() appropriately.

It seems that what you're suggesting is that instead of defining a wrapper class that you can use if you want to mock the file access, each class should define separate methods on an ad hoc basis to wrap the file access. Wouldn't it be preferable to define the wrappers only once?

If we wanted, we could have the wrappers be overridden statically rather than being injected, in the style of ConvertibleTimestamp::setFakeTime(). That would avoid having to inject them in each class.

@Simetrical Disabling access for the case of "doesn't depend on FS" seems useful, and could perhaps be mimicked with a vfsStream mock that has deleted the file it would read and/or give it a read-only empty dir.

But the general use case of asserting specific FS access seems to me like an implementation detail that doesn't belong in unit tests. It might sound odd, but I don't think it's part of the contract for a caching class to.. cache something. We often have NullThing/EmptyThing implementations that still fulfil the contract. Especially in the realm of Memcached, I've sometimes found bugs by giving a piece of code EmptyBagOStuff and finding code that wrongly assumed a server is always available to store something (T203786, T208934) or to otherwise assume values remain at least between two nearby moments in time.

This is getting rather specific about LC, but I'd say if you modified a source file, and LC picks it up, it ignored the cache (or had none). If you modified the source file, and it didn't pick it up, you know it decided to use a cache. Those kinds of observable details are what I tend to test.

For the general case of preventing some methods from being called, or stubbing their dependency on an out-of-test dependency (such as HTTP, DNS, or indeed the file system) - it is indeed common to allow stubbing those out by creating a one-line method for them. That is how we've usually done that in MediaWiki as far as I know. And it's a nice general method that works for things beyond filesystem as well.

It is not part of the contract of a cache to cache something, insofar as caches by definition are for things that can be regenerated, and therefore are not designed to guarantee persistency.

However, it is part of MediaWiki performance requirements that caches are being used properly, and that should be tested. If something is supposed to be serving everything from cache and is instead hitting the filesystem every time because a refactor introduced a logic error, that is a bug and tests should catch it.

The fact that the cache may sometimes not return a value does not mean that we don't need to test that the user of the cache is behaving correctly when the cache does return a value. The test of the class that uses the cache should supply a mock cache that does return a usable value, and test that the class doesn't still do the expensive work the cache is supposed to avert.

If the consensus is that this should be done via a one-line method to allow test stubbing, then I'm okay with that. If enough tests have these one-line methods that it's worth refactoring them to use a common method, I'll bring this up again.