Skip to content

[AssetMapper] Surround brittle test teardown with try/catch #57346

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

Closed

Conversation

alexandre-daubois
Copy link
Member

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix AppVeyor
License MIT

AppVeyor fails regularly when tearing down RemotePackageStorageTest. I think it is acceptable to surround it with a try/catch, because everything will be erased in AppVeyor once the test suite is ran.

@xabbuh
Copy link
Member

xabbuh commented Jun 7, 2024

If I don't miss anything, this failure reveals an issue in the behaviour of the Filesystem component on Windows (I started debugging this in #54762). I am fine with doing this as a workaround for now, but we somehow need to keep track of the issue nonetheless and try to fix it.

@alexandre-daubois
Copy link
Member Author

I'm surprised there's a bug in Filesystem and it only appeared now 😮 My first impression is that it is linked to dumpFile(), which creates a temp file that doesn't seem to be removed when being renamed at the end of Filesystem::dumpFile(). I don't know if you explored this?

@smnandre
Copy link
Member

smnandre commented Jun 7, 2024

Could it be some race conditions between tests ? As the other failing tests on this PR CI checks are also FileSystem linked to file permissions... and never happened IRL ?

Update: could we try the same code as before but remove the parralel runs ?

@alexandre-daubois
Copy link
Member Author

It seems that chmod is involved. Here's what I wrote to @xabbuh :

Windows doesn't support using chmod, which is done in the "testSaveThrowsWhenVendorDirectoryIsNotWritable" test case. chmod is the command that creates the weird directory that starts with "._". It's super strange, but yeah, Windows doesn't support granular permissions and uses some other ACL things.

I have no better idea than disabling the related test for Windows, so we're still discussing what's the best way to solve this before ignoring anything 🙂

@smnandre
Copy link
Member

Just discovering Filesystem tests use a markAsSkippedIfChmodIsMissing() method for this very same reason..

    protected function markAsSkippedIfChmodIsMissing()
    {
        if ('\\' === \DIRECTORY_SEPARATOR) {
            $this->markTestSkipped('chmod is not supported on Windows');
        }
    }

Example in the testRemoveThrowsExceptionOnPermissionDenied method

    public function testRemoveThrowsExceptionOnPermissionDenied()
    {
        $this->markAsSkippedIfChmodIsMissing();

        $basePath = $this->workspace.\DIRECTORY_SEPARATOR.'dir_permissions';
        mkdir($basePath);
        $file = $basePath.\DIRECTORY_SEPARATOR.'file';
        touch($file);
        chmod($basePath, 0400);

        try {
            $this->filesystem->remove($file);
            $this->fail('Filesystem::remove() should throw an exception');
        } catch (IOException $e) {
            $this->assertStringContainsString('Failed to remove file "'.$file.'"', $e->getMessage());
            $this->assertStringContainsString('Permission denied', $e->getMessage());
        } finally {
            // Make sure we can clean up this file
            chmod($basePath, 0777);
        }
    }

@smnandre
Copy link
Member

@alexandre-daubois I opened a PR because I did not see any other way for me to check on Windows.
Feel free to close it or change the author.

I don't want to appear impolite by opening another one, especially when it's my code at fault here 🫨

@smnandre
Copy link
Member

The remaining one seems to me like it's really a bug in filesystem on windows...

Could this be related to this change, with windows handling of "dotfiles" ?
#49581

// BEFORE

echo \dirname(realpath($file)).'/.'.strrev(strtr(base64_encode(random_bytes(2)), '/=', '-.'));
// ex: /tmp/..AGk

// --> iIgnored by the iterator FilesystemIterator::SKIP_DOTS in order to remove its child before itself


// AFTER

echo \dirname(realpath($file)).'/.'.strrev(strtr(base64_encode(random_bytes(2)), '/=', '-_'));
// ex: /tmp/._IC2

// --> Not ignored or wrong order ?

@alexandre-daubois
Copy link
Member Author

I don't want to appear impolite by opening another one, especially when it's my code at fault here

That's totally fine 😄 It seems that only the cleanup part fails, but the rest of the testSaveThrowsWhenVendorDirectoryIsNotWritable case actually "works" and raises the expected exception. The question is : should the test be totally ignored? I don't have a strong opinion here. What you propose in #57375 suits me fine also. If it's merged, let's close this one !

Could this be related to this change, with windows handling of "dotfiles" ?

Would definitely worth a look, I'll give it a deeper look soon 👍

@smnandre
Copy link
Member

The remaining one seems to me like it's really a bug in filesystem on windows...

Could this be related to this change, with windows handling of "dotfiles" ? #49581

This thread seems to confirm it (or at least give some weight) .. as now some folder would be handled before files in it if i read it correctly

Alpha order of Windows seems to be:

..
.-
._

image

(image taken from the stackoverflow thread)

Seeing the algorithm in the remove() method, i'm 99% sure the linked change has an impact on Windows... that we can feel on these tests.

@nicolas-grekas
Copy link
Member

Let's try #57574

nicolas-grekas added a commit that referenced this pull request Jun 28, 2024
…grekas)

This PR was merged into the 5.4 branch.

Discussion
----------

[Filesystem] Fix Filesystem::remove() on Windows

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | #57375, #57346
| License       | MIT

Let's experiment with `@smnandre`'s theory.

Commits
-------

2b50dea [Filesystem] Fix Filesystem::remove() on Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants