Contrib.social

Remove all fixtures except for one: `fake_site`

Open on Drupal.org →
Created on 27 October 2022, about 2 years ago
Updated 1 February 2023, almost 2 years ago

Problem/Motivation

A little while ago, @tedbow, @TravisCarden, and I talked about how some of our test fixtures have grown out-of-control. In particular, tests which rely on separate "active" and "staged" fixtures are very hard to understand, since you have to know what each fixture represents, and how they relate to the test(s) they're coupled with.

To solve this, we added three new methods to FixtureUtilityTrait: addPackage(), removePackage(), and modifyPackage(). (See #3317385: Remove staged fixtures from StagedProjectsValidatorTest → ). Then, we used these new methods to make StagedProjectsValidatorTest completely fixture-less in that issue, and in #3317796: Make StagedProjectsValidatorTest fixture-less → .

Proposed resolution

Currently we are only able to remove the composer fixture files, not the module/theme fixture files.

Let's do the same thing for other tests which follow a similar pattern. In particular, I think we could convert the following, using the new StagedProjectsValidatorTest as a model:

  • \Drupal\Tests\package_manager\Kernel\OverwriteExistingPackagesValidatorTest
  • \Drupal\Tests\automatic_updates_extensions\Functional\UpdaterFormTest. In particular, the following fixtures probably do not need to exist at all:
    • no_project: This simulates an active directory with no non-core projects installed; why can't we use Package Manager's fake_site -- the default basis for any virtual project -- for this?
    • one_project: Could we replace this with a single call to FixtureUtilityTrait::addPackage()?
    • stage_composer/two_projects: This also looks suspiciously like it could be replaced with a couple of calls to addPackage().

For each of these test classes, we should probably open a new child issue and refactor it to use the new stuff in FixtureUtilityTrait, if we can. The more of our fixtures we can remove, the better off we'll be.

We should first try to convert the kernel tests because the functional tests will need more work get the stage directory call functions in FixtureUtilityTrait

End goal

  1. Our fixtures should have our tests/fixtures folders should have no installed.json, installed.php

    The exceptions to this are package_manager/tests/fixtures/fake_site and automatic_updates_extensions/tests/fixtures/fake-site which are our starting point fixtures

  2. Our tests should not have any *.info.yml.hide files.(we had to use the .hide prefix because drupalci complains if info.yml files have the project key set

    #3321236: Add actual project folders in \Drupal\Tests\package_manager\Traits\FixtureUtilityTrait::addPackage → will have to completed to enable this

  3. The only exception to the 2 points above may be if we need to create fixtures that are set up incorrectly specifically to test validation that detects that incorrect setup. It may make more sense to leave a few fixture files than to make our methods in FixtureUtilityTrait flexible enough create fixtures that we don't consider "correct". An example of this would be if we needed to create installed.json and installed.php files for single use case where the packages in these 2 files did match. In a valid composer setup these packages would be the same in these 2 files.
📌 Task
Status

Fixed

Version

2.0

Component

Code

Created by

🇺🇸United States phenaproxima Massachusetts

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Assigned to omkar.podey
  • Status changed to Needs work almost 2 years ago
  • Assigned to wim leers
  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇮🇳India omkar.podey
  • Assigned to tedbow
  • Status changed to RTBC almost 2 years ago
  • Comment almost 2 years ago →
    🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    This is ready AFAICT, and the documentation helps a lot. But I think the documentation could be far clearer still.

    I left 4 suggestions: 2 of which are trivial Markdown tweaks, the other 2 are providing more context to the documentation that was already being added), and one also contains a question that may require the creation of a follow-up issue.

    Assigning @tedbow for accepting and merging my suggestions, but this is definitely in the RTBC realm :)

  • Comment almost 2 years ago →
    System Message
  • Status changed to Fixed almost 2 years ago
  • Comment almost 2 years ago →
    🇺🇸United States tedbow Ithaca, NY, USA

    Merged because the last change from gitlab was just to readme. So don't need to wait on tests 🤞🏻(will check 8.x-2.x)

    thanks!

  • Comment almost 2 years ago →
    🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    You did not respond to:

    ⇒ this actually makes me realize that BuildTests should never use (Active|Stage)FixtureManipulator? 🤔 Because doing so means we're no longer testing php-tuf/composer-stager

Thoughts, @tedbow?

    → that is what could merit a follow-up 😊

  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇺🇸United States tedbow Ithaca, NY, USA

    @Wim Leers sorry. Yes I agree with the comment that (Active|Stage)FixtureManipulator should not be used in build tests.

    The only case I could see were we might use it is if used `\Drupal\fixture_manipulator\FixtureManipulator::addProjectAtPath` to the active directory to simulate having a project that is not known to composer. But we haven't had a need for that yet.

    that is what could merit a follow-up 😊

    I follow up to make the comment more specific or to somehow forbid this

    AFAICT we not using (Active|Stage)FixtureManipulator in build tests right now. Did you find a case where we are?

  • Comment almost 2 years ago →
    🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Did you find a case where we are?

    No, I did not. But we were contemplating it in this very issue — that is literally what the docs we just committed (and you wrote the key parts of it in #14) are saying: "we can't use the manipulator for reasons X and Y" — which wouldn't even be a consideration if build tests could not use the manipulators!

  • Comment almost 2 years ago →
    🇺🇸United States tedbow Ithaca, NY, USA

    I created 🐛 Document that StageFixtureManipulator should not be used in build tests Active as a core-post-mvp lets move discussion there and close this

  • Issue was unassigned.
  • Status changed to Fixed almost 2 years ago
  • Comment almost 2 years ago →
    🇺🇸United States tedbow Ithaca, NY, USA
  • Comment over 1 year ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024