Skip to content
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

[6.x] Fix Filesystem tests failing in Windows #32975

Merged
merged 1 commit into from
May 26, 2020

Conversation

derekmd
Copy link
Contributor

@derekmd derekmd commented May 26, 2020

This should be the last of it. All tests are now green in Windows.

FilesystemAdapterTest::testPutWithStreamInterface()

#30179 comments noted that Storage::put() implementation could leave a stream open by the PHP script. This has caused a chain of filesystem tests to fail in Windows because FilesystemAdapterTest::tearDown() can't delete the temp directory when a "foo.txt" temp file has an active fopen() stream.

  • GuzzleHttp\Psr7\Stream::__destruct() is meant to gracefully fclose($this->stream) through GuzzleHttp\Psr7\Stream::__close()
  • Having the framework call GuzzleHttp\Psr7\Stream::detach() to fetch stream content, the close() method now hits $this->stream === null so it's on userland to explicitly call fclose().

FilesystemTest::testReplaceStoresFiles()

#26254 added a test that only works in Linux/macOS. The chmod() / umask() write-permission changes have no effect on Windows and symlink() fails unless:

  • the original file "{$this->tempDir}/file.txt" already exists.
  • you're running PHPUnit using an Administrator role - at least this is true under Git Bash / MINGW64 shell.

So I've skipped adding the below Windows-only test as a counterpart to testReplaceWhenUnixSymlinkExists.

    public function testReplaceWhenWindowsSymlinkExists()
    {
        if (! windows_os()) {
            $this->markTestSkipped('Skipping since operating system is not Windows');
        }

        $tempFile = "{$this->tempDir}/file.txt";
        file_put_contents($tempFile, 'Hello World');
        $symlinkDir = "{$this->tempDir}/symlink_dir";
        $symlink = "{$symlinkDir}/symlink.txt";

        mkdir($symlinkDir);
        symlink($tempFile, $symlink);

        $filesystem = new Filesystem;

        // Test replacing existing file.
        $filesystem->replace($tempFile, 'Something Else');
        $this->assertStringEqualsFile($tempFile, 'Something Else');

        // Test replacing symlinked file.
        $filesystem->replace($symlink, 'Yet Something Else Again');
        $this->assertStringEqualsFile($tempFile, 'Yet Something Else Again');
    }

windows-tests

* testPutWithStreamInterface() leaves an fopen()
  stream resource that must be explicitly closed
  before temp directories can be deleted in
  Windows.
* testReplaceStoresFiles() fails for multiple
  reasons under Windows. chmod() / umask() write
  permissions don't change and symlink() attempts
  fail in most Windows environments.
@driesvints
Copy link
Member

@derekmd That's great. Might be worth to flip the switch on parallel builds in Github actions?

@derekmd
Copy link
Contributor Author

derekmd commented May 26, 2020

@driesvints: For my 100% pass test run on desktop some cases were skipped from PHP extensions not being available. Specifically cases outputting these messages:

  • Memcached module not installed
  • PHP was compiled without PDO ODBC support.
  • DynamoDB not configured.
  • PhpRedis does not support persistent connections with PHP_ZTS enabled.

If those PHP extensions are installed in CI and pass, the framework should be good to introduce Windows to the parallel builds.

The 6.x and 7.x branches are good. I'll have a look at master / 8.x once these pull requests are merged up.

@taylorotwell taylorotwell merged commit cfed253 into laravel:6.x May 26, 2020
@derekmd derekmd deleted the fix-filesystem-tests-on-windows branch May 26, 2020 20:36
@derekmd
Copy link
Contributor Author

derekmd commented May 30, 2020

@driesvints: I confirmed the three active branches have tests passing in Windows.

  • 6.x
  • 7.x
  • master for 8.x

Should I look into a branch to add a Windows config to /.github/workflows (maybe just one matrix entry for PHP 7.4) or should I leave that to the core team?

@driesvints
Copy link
Member

Feel free to already send in a PR. I'll try to have a look early next week.

@derekmd
Copy link
Contributor Author

derekmd commented Jun 1, 2020

@driesvints: It looks like limitations in GitHub Actions may prevent Windows from being added right now, although I was able to get it passing 97% of the framework's tests.

Tests: 4938, Assertions: 11374, Errors: 20, Skipped: 117.
  1. Service containers only work for the Linux environment so setup for MySQL, Redis, and Memcached won't work in Windows.

    • PHPUnit tests requiring Redis & Memcached extensions are silently skipped so this edge case is not a big deal.
  2. The Windows Server 2019 instance has the MySQL client pre-installed but doesn't seem to have a server. https://github.com/actions/virtual-environments/blob/master/images/win/Windows2019-Readme.md

    Mysql

    Version: 5.7.21.0
    Environment:

    PATH: contains location of mysql.exe

    My tests.yml config attempt using below couldn't find executable mysqld so I assume the server isn't installed.

          - name: Start MySQL server
            run: start /b cmd /c mysqld.exe > "NUL" 2>&1
            env:
              MYSQL_ALLOW_EMPTY_PASSWORD: yes
              MYSQL_DATABASE: forge
    

The 20 failing tests are all MySQL-related. Self-hosted GitHub Actions runners are out of scope (and costly) so the fix to get green would be either:

  1. Call $this->markTestSkipped() on Windows when the local MySQL server is unavailable.
  2. Run an external MySQL server outside of GitHub Actions' virtual environment that is located in the same geographic region. https://help.github.com/en/actions/reference/virtual-environments-for-github-hosted-runners#ip-addresses-of-github-hosted-runners shows all instances are in the US so a Chicago/Austin MySQL instance would be the best-guess location. I assume this solution is also too costly.

@driesvints
Copy link
Member

Think skipping the MySQL tests might not be the worst idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants