Problem/Motivation

Realized that #3315834: GitExcluder should not ignore .git directories that belong to packages installed by Composer's new test additions are leaving behind stage directories in the staging root. It's very unlikely to result in collisions. But it can accumulate quite a bit of disk space in principle.

Let's clean up behind us.

Until #3328234: Improve test DX *and* confidence: stop using VFS this wasn't necessary, because the staging root was always inside VFS, but #3328234 will change that.

Steps to reproduce

See above.

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#2 3334552-2.patch1.35 KBwim leers
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

StatusFileSize
new1.35 KB
tedbow’s picture

Title: [PP-2] Ensure that intentionally failed test stage directories are deleted after tests complete » [PP-1] Ensure that intentionally failed test stage directories are deleted after tests complete

I think this issue was postponed on #3328234: Improve test DX *and* confidence: stop using VFS which just got committed

wim leers’s picture

Indeed! 🥳

wim leers’s picture

Title: [PP-1] Ensure that intentionally failed test stage directories are deleted after tests complete » [PP-2] Ensure that intentionally failed test stage directories are deleted after tests complete

#3315834: GitExcluder should not ignore .git directories that belong to packages installed by Composer is postponed again on another issue, so matching that increase in blockers.

tedbow’s picture

wim leers’s picture

Title: [PP-2] Ensure that intentionally failed test stage directories are deleted after tests complete » Ensure that intentionally failed test stage directories are deleted after tests complete
tedbow’s picture

Version: 8.x-2.x-dev » 3.0.x-dev
yash.rode’s picture

Assigned: Unassigned » yash.rode
Status: Active » Needs work
yash.rode’s picture

Before starting with this I have a thought about how can we achieve this,
one way is to check each and every test and destroy the stage where it is not destroyed already.
the other way can be using tearDown() but I don't know how can we get the stage in that case?

yash.rode’s picture

Issue tags: +sprint
yash.rode’s picture

As discussed in meet one ways to do this are,

  1. In assertResults() we can add a finally block and delete stage in that
  2. In teardown we can delete the stage directory.

question about the teardown approach, can we do that in the PackageManagerKernelTestBase and other testBases also?

tedbow’s picture

#12
I think we want to go with 2)

but slightly different in tearDown() we should delete `\Drupal\package_manager\PathLocator::getStagingRoot()`
and \Drupal\package_manager\PathLocator::getProjectRoot() but we should only delete getProjectRoot() if is directory that is under `DrupalFileSystem::getOsTemporaryDirectory()` which will be the case if it is created in `\Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::createTestProject`

phenaproxima’s picture

and \Drupal\package_manager\PathLocator::getProjectRoot() but we should only delete getProjectRoot() if is directory that is under `DrupalFileSystem::getOsTemporaryDirectory()` which will be the case if it is created in `\Drupal\Tests\package_manager\Kernel

I think we could probably simplify this; if the project root is not the same as $this->getDrupalRoot(), we can delete it. I think this is the case because, in a core clone, $this->getDrupalRoot() is what the path locator will report as the project root, by default, unless we specifically override it.

yash.rode’s picture

Assigned: yash.rode » tedbow

Need help with the test failure, \Drupal\Tests\package_manager\Kernel\SymlinkValidatorTest::testSymlinkToDirectory is failing with rmdir(/private/tmp/package_manager_testing_roottest95214460/active): Directory not empty, is it because we are trying to delete symlink? or something else?

yash.rode’s picture

Assigned: tedbow » Unassigned
Status: Needs work » Needs review

Right now I am skipping \Drupal\Tests\package_manager\Kernel\StageOwnershipTest::testStageDestroyedWithFileSystemError because we are overriding chmod(), unlink(), rmdir() which we need in tearDown(), is there a way to not skip this test?

phenaproxima’s picture

Status: Needs review » Needs work

We shouldn't use the Drupal file_system service to do the delete, precisely because, in some situations, we override and mock it. Instead, we should use Symfony's Filesystem class, which has a remove() method which can do this for us cleanly.

yash.rode’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Assigned: Unassigned » yash.rode
Status: Needs review » Needs work

This looks straightforward but there are some things there that I don't understand why they're necessary...

yash.rode’s picture

Assigned: yash.rode » Unassigned
Status: Needs work » Needs review

Answered questions and removed the unlink() calls.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima
Status: Needs review » Needs work

I don't think we want to be modifying permissions of the system-wide temp directory.

If NOT doing that causes some tests to fail, I'd say that points to a flaw in those tests. I will take this on to figure out why we'd need to do that, and fix the affected tests.

yash.rode’s picture

I am not able to post comment on GitLab, this is not true I guess, example$staging_root will be /private/tmp/package_manager_testing_roottest30218963/stage so it's parent won't be temp directory.

phenaproxima’s picture

Assigned: phenaproxima » tedbow
Status: Needs work » Needs review

Assigning to @tedbow for final review.

tedbow’s picture

Assigned: tedbow » phenaproxima
Status: Needs review » Reviewed & tested by the community

This looks good. I pushed up 1 commit because there was 1 folder not being deleted. @phenaproxima if this looks good and tests pass, feel free to commit.

phenaproxima’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.