Problem/Motivation

Automatic Updates' build tests are extremely valuable, and utterly unmaintainable. They are way too complicated, and spend a lot of time setting up and asserting low-level things they have no business asserting. They are, in a word, overzealous and incredibly awkward due to the way they were written (to work both in the context of a regular contrib project, and in the context of a core clone).

When they fail, it is for the strangest of reasons and can only be debugged by me.

Proposed resolution

Rewrite them so that they set up a single coherent fixture, and stop asserting low-level stuff that they shouldn't care about. These are end-to-end tests, let's treat them that way.

This will mean removal of test coverage. It will obviously remain in the Git history if we need to refer to it later, but in general there are some things that these tests should not cover. I will document, as I go, the decisions I make about what to rip out.

Issue fork automatic_updates-3582495

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:

About issue forks

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

I have decided to remove \Drupal\Tests\automatic_updates\Build\ApiCoreUpdateTest entirely.

This is effectively a low-level test that, for whatever reason, is implemented as a high-level test. Its main benefit is that it confirms that updates are prevented if the filesystem is not writable...but that's already confirmed by \Drupal\Tests\package_manager\Kernel\WritableFileSystemValidatorTest. I am not sure that the functional test coverage of the build test is all that valuable, because this really just boils down to checking that is_writable() is called, and the kernel test in core confirms that just fine.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

I've also decided to remove the similar test from ModuleUpdateTest. It's really pointless; it's testing that you can use a test-only HTTP API to update a module -- something that will never happen in real life because there is no actual production HTTP API that can do updates.

The test already covers updating a module in the UI anyway, so this method doesn't really have any reason to be there.