Closed (fixed)
Project:
Automatic Updates
Version:
3.0.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Issue tags:
Reporter:
Created:
18 Jan 2023 at 13:10 UTC
Updated:
19 Apr 2023 at 21:19 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
wim leersComment #3
tedbowI think this issue was postponed on #3328234: Improve test DX *and* confidence: stop using VFS which just got committed
Comment #4
wim leersIndeed! 🥳
Comment #5
wim leers#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.
Comment #6
tedbow#3315834: GitExcluder should not ignore .git directories that belong to packages installed by Composer was fixed🎉
Comment #7
wim leersComment #8
tedbowComment #9
yash.rode commentedComment #10
yash.rode commentedBefore 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?
Comment #11
yash.rode commentedComment #12
yash.rode commentedAs discussed in meet one ways to do this are,
question about the teardown approach, can we do that in the
PackageManagerKernelTestBaseand other testBases also?Comment #13
tedbow#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`
Comment #14
phenaproximaI 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.Comment #16
yash.rode commentedNeed help with the test failure,
\Drupal\Tests\package_manager\Kernel\SymlinkValidatorTest::testSymlinkToDirectoryis 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?Comment #17
yash.rode commentedRight now I am skipping
\Drupal\Tests\package_manager\Kernel\StageOwnershipTest::testStageDestroyedWithFileSystemErrorbecause we are overriding chmod(), unlink(), rmdir() which we need in tearDown(), is there a way to not skip this test?Comment #18
phenaproximaWe 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.Comment #19
yash.rode commentedComment #20
phenaproximaThis looks straightforward but there are some things there that I don't understand why they're necessary...
Comment #21
yash.rode commentedAnswered questions and removed the unlink() calls.
Comment #22
phenaproximaI 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.
Comment #23
yash.rode commentedI am not able to post comment on GitLab, this is not true I guess, example
$staging_rootwill be/private/tmp/package_manager_testing_roottest30218963/stageso it's parent won't be temp directory.Comment #24
phenaproximaAssigning to @tedbow for final review.
Comment #25
tedbowThis 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.
Comment #27
phenaproxima