Problem/Motivation

There are tests that use the state service to determine whether a value was set as expected. Many of these usages are not testing the state service specifically, such as in #3477586: [random test failure] LayoutBuilderBlocksTest::testBlockPlaceholder failing . There are race conditions associated with state that can cause intermittent test failures. While the race conditions are being addressed in #3496257: Race conditions in CacheCollector/State (again), using state in this cases should be replaced using key value storage directly.

This issue is to identify all the instances of stage usage that can be replaced. If the number of instances is small, then likely replacing them can all be done here as well. Or perhaps this issue can be converted to a meta with separate child issues to replace individual instances to break the work up.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Comments

godotislate created an issue. See original summary.

quietone’s picture

quietone commented
Related issues: +#3437176: Improve documentation of the state and key value API
catch’s picture

catch
he/him
Primary language English
commented

EntityComputedFieldTest has a random failure that looks like it's due to state usage:

https://git.drupalcode.org/project/drupal/-/jobs/5355698

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

catch’s picture

catch
he/him
Primary language English
commented
Title: Identify usages in tests of State that can be replaced with key value instead » [meta] Replace all uses of state in tests with direct key value
Category: Task » Plan
Priority: Normal » Major

Updating the issue title, we unless we're actually testing the state system we shouldn't be using it in test modules etc.

joachim’s picture

joachim commented

What??

State is a really useful way for Kernel tests to set things for fixture code to pick up. The entity_test module uses it LOADS:

    $bundles = \Drupal::state()->get($entity_type . '.bundles', [$entity_type => ['label' => 'Entity Test Bundle']]);

    $views_data = NestedArray::mergeDeep($views_data, \Drupal::state()->get('entity_test.views_data', []));

    return $fields + \Drupal::state()->get($entity_type->id() . '.additional_base_field_definitions', []);

    foreach (\Drupal::state()->get('entity_test_reference_computed_target_ids', []) as $delta => $id) {

// and many more
catch’s picture

catch
he/him
Primary language English
commented

@joachim, see the issue title:

Replace all uses of state in tests with direct key value

Also #3496257: Race conditions in CacheCollector/State (again).

joachim’s picture

joachim commented

I saw. I'm saying this is going to have a massive impact across tests -- lots of contrib tests will be using those test entity types too. And that it's a bit ridiculous that state is broken like this.

And how is it going to be enforced / documented not to use state in tests for passing/stashing information?

catch’s picture

catch
he/him
Primary language English
commented

And that it's a bit ridiculous that state is broken like this.

#3496257: Race conditions in CacheCollector/State (again) has an MR that needs review.

And how is it going to be enforced / documented not to use state in tests for passing/stashing information?

It won't be enforced, some tests need to test the actual state system. Core can lead by example though. I don't see how updating some tests is worse than the tests failing randomly.

godotislate’s picture

godotislate
he/him
commented

Since #3502432: Make hook testing with kernel tests very simple and #3390193: Add a drupalGet() method to KernelTestBase, there are probably a bunch of tests where we can eliminate the use of storing things in state/keyvalue at all. Hook implementations that live inside the Kernel test class can have properties that are set and checked, instead of stored values in the database. And moving some functional tests to kernel tests might make those tests eligible for the same treatment.