Move tour tests to the tour module

Moved some tests and removed tour from the test module. Let's see what fails.

The last submitted patch, 2: 3336050-2.patch, failed testing. View results →

Postponing as the policy does not have consensus yet.

The removal has been approved.

Traded out tour with taxonomy because it had 2 actions. 1 to be install and other to be optional.

The failing test actually passes locally but maybe it needs to be removed from standard?

@smustgrave, thanks for working on this! Removing an extension from a profile is better handled in a separate issue, a child issue of the parent. That is what was done for RDF, Color, Bartik and Seven. The parent meta tries to point that out in Step 2.

I was curious about the test failure in #7. It was a namespace error. So, I rolled a patch without any profile related changes.

Woo all green. That test was confusing me glad you figured it out.

Going to mark this but only area I was iffy about that I did.

-    // language tour has a dependency on this tour so it has to exist.
-    $this->assertInstanceOf(Tour::class, Tour::load('testing_config_overrides_module'));
-
-    // Ensure that optional configuration from a profile is created if
-    // dependencies are met.
-    $this->assertEquals('Config override test', Tour::load('testing_config_overrides')->label());
+    // taxonomy action has a dependency on this setting so verify it exists.
+    $this->assertNotNull($this->config('testing_config_overrides_module.settings'));

The last submitted patch, 8: interdiff-7-8.patch, failed testing. View results →

We will still have to figure out what to do with \Drupal\Tests\config\Functional\ConfigInstallProfileOverrideTest because it has some Tour related tests.

What about this?

The last submitted patch, 14: 3336050-14.patch, failed testing. View results →

Updated namespace to Jsonapi

The last submitted patch, 17: 3336050-17.patch, failed testing. View results →

Changed the wrong file in #17 lets try this.

All green!

For my review, I read the code. I did not run any tests.

1. +++ b/core/modules/config/tests/src/Functional/ConfigInstallProfileOverrideTest.php
   @@ -87,18 +86,16 @@ public function testInstallProfileConfigOverwrite() {
        // Ensure that optional configuration can be overridden.
   ...
   +    // Should not be overridden because it's optional and setting exists in
   +    // taxonomy module already.
   

   I am not following this block. It is supposed to be test that optional config can be overridden.

2. +++ b/core/modules/tour/tests/src/Functional/ViewsUi/ViewsUITourTest.php
   --- /dev/null
   +++ b/core/profiles/testing_config_overrides/config/install/system.action.taxonomy_term_publish_action.yml
   
   +++ b/core/profiles/testing_config_overrides/config/optional/config_test.dynamic.override.yml
   --- /dev/null
   +++ b/core/profiles/testing_config_overrides/config/optional/system.action.taxonomy_term_unpublish_action.yml
   

   I see that, these files have a very different order than the others in the same directory. And they have an extra property as well. I think these should follow the same structure as the others fore maintainability.

   We should also change the id and text so that it is obvious that these are testing values. When I first saw that related code I went looking at the taxonomy module instead of the testing module.

Looking at the history here I think this moved to RTBC to soon. We all need to remember that a patch needs to be reviewed before RTBC, not self-RTBC ed, and to explain why you think an issue qualifies for that status in your comment. The passing of tests is rarely sufficient. I made the initial patch and I did not review the work done after that. Yes, in #10 I made a change but I did not review. In fact, the first review of the patch was done in #11 by a core committer. Core committers tend to focus on the RTBC queue, wanting issues to be ready for commit.

But yea, we all make mistakes. :-)

The test is checking configuration import. Taxonomy is the closest thing I could find to replace tour config

@quietone just following up on what should be changed if anything?

@smustgrave asked for my feedback on this issue.

In this situation we're up against a test that was not well-designed in the first place. In general, tests should not be coupled to specific core features; instead, independent test module fixtures with the needed structure should be added and used in the tests. So, unfortunately, I think we need to go down the route of adding possibly two new test modules that have the needed configuration structure for this. (We can also check for existing test modules that would fit.)

Rewriting the test could be moved to a separate issue, so that all this issue has to do is move the non-problematic tests.

Opened https://www.drupal.org/project/drupal/issues/3387163 📌 Decouple tour from ConfigInstallProfileOverrideTest Needs review

This patch just includes moving the tests.

Fixing attribution.

Nice small set of changes now, great stuff!

Committed dce6022 and pushed to 11.x. Thanks!

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