Problem

Details

  • EnableDisableTest must not test any non-core modules that may exist.

Proposed solution

  1. Change EnableDisableTest to retrieve all available modules and filter them based on their filesystem path instead of the package name.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +language-base

"Change EnableDisableTest to retrieve all available modules and filter them based on their filesystem path instead of the package name." sounds like a good plan, agreed.

YesCT’s picture

Issue tags: +medium

I almost made this novice, but sounds a bit more complicated than novice. Marking medium, since we can use d8mi plus medium to make a focus board for "good one to jump in too".

vijaycs85’s picture

Status: Active » Needs review
FileSize
822 bytes

Giving a try.

Status: Needs review » Needs work

The last submitted patch, 1871328-1.patch, failed testing.

vijaycs85’s picture

Assigned: Unassigned » vijaycs85
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.67 KB

Sorry for that TDD approach. updating test cases to get package dynamically.

tstoeckler’s picture

It seems this is missing the check on the filepath, no?

vijaycs85’s picture

oops...good catch tstoeckler

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Module/EnableDisableTest.php
@@ -27,7 +27,7 @@ function testEnableDisable() {
+      if (strpos($module->filename, 'core/modules') !== 0 || !empty($module->info['hidden']) || !empty($module->info['required'])) {

I think the strict type comparison should be done with FALSE instead of 0, and also I think the condition is reversed. We want to unset modules that are *not* provided by core, i.e. modules that do *not* have 'core/modules' in their filename, i.e. === FALSE. It's late/early here, so I might be completely nuts.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

@tstoeckler: it depends on if the filename starts off with 'core/modules', then the check passes if the position is 0, so the strict type comparison is correct when its a core module. I believe that is what the patch intends to do. Otherwise it would pass even if there is a contrib package named encore let's say with submodules in a modules directory, so it would be under modules/encore/modules/... matching core/modules.

I've looked at the code, and it seems all right to me, assuming the above strpos() check is correct. We probably need a code comment right above it explaining it clearly so others don't get tripped up by it.

Gábor Hojtsy’s picture

Issue tags: +sprint
Gábor Hojtsy’s picture

Ok, that assumption does not seem to be true :)

vijaycs85’s picture

Not sure where these modules are coming from:
requirements1_test
requirements2_test
system_dependencies_test
system_incompatible_core_version_dependencies_test
system_incompatible_core_version_test
system_incompatible_module_version_dependencies_test
system_incompatible_module_version_test

and why didn't they break before :(

Gábor Hojtsy’s picture

Looks like hidden test modules. Lots of them are in core:

- core/modules/system/tests/modules/requirements1_test
- core/modules/system/tests/modules/requirements2_test

And so on.

sun’s picture

Let's also add a new assertion directly after that initial foreach-loop/filtering to verify that the resulting/filtered list is not empty.

The new strpos() condition does not filter out testing modules anymore, which means that the test now tries to enable modules in the "Testing" package (which is not exposed on the Modules page). All of those modules are marked as hidden = TRUE.

The reason for that is how PHP evaluates expressions, and strpos() in particular frequently trips up developers.

There are two ways to make the condition work as intended:

- if (strpos($module->filename, 'core/modules') !== 0 || !empty($module->info['hidden']) || !empty($module->info['required']))
+ if (0 !== strpos($module->filename, 'core/modules') || !empty($module->info['hidden']) || !empty($module->info['required']))
+ if ((strpos($module->filename, 'core/modules') !== 0) || !empty($module->info['hidden']) || !empty($module->info['required']))
vijaycs85’s picture

For some reasons, hidden modules getting just name in $module->filename(e.g. requirements1_test.module) instead of whole path(core/modules/system/tests/modules/requirements1_test.module). So changing condition to use $module->uri.

vijaycs85’s picture

cross post... updating fix in #15

Status: Needs review » Needs work

The last submitted patch, 1871328-17-test-case-change.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

Though strpos() does something funny, all these hidden modules should caught by 2nd part of the condition ...

YesCT’s picture

Status: Needs review » Needs work

I used devel, and looked.

  $modules = system_rebuild_module_data();

Here requirements1_test is in the $modules array.

    foreach ($modules as $name => $module) {
      if ((strpos($module->uri, 'core/modules') !== 0) || !empty($module->info['hidden']) || !empty($module->info['required'])) {
        unset($modules[$name]);
      }
    }

At this point, requirements1_test, etc is NOT in the $modules array. So the condition does work...
but then I think the next part leads me to think it might be getting added back in...

    // Throughout this test, some modules may be automatically enabled (due to
    // dependencies). We'll keep track of them in an array, so we can handle
    // them separately.
    $automatically_enabled = array();
YesCT’s picture

I cannot find this assert message:

$this->assertTrue(count($modules), format_string('Found @count modules that can be enabled: %modules', array(
      '@count' => count($modules),
      '%modules' => implode(', ', array_keys($modules)),
    )));

I searched for "that can be enabled".

The first one I see is from:

$this->assertText(t('The configuration options have been saved.'), 'Modules status has been updated.');
vijaycs85’s picture

Status: Needs review » Needs work
FileSize
32.22 KB

YesCT, attached my local test result with this message. It looks like somewhere the "hidden" property of these modules getting removed only for simpletest. As the name stands, it make sense that these are valid modules to cover test cases. we might need to find a way to get a way with them. i.e. have them visible, but should skip enable/disable case.

how about required = TRUE with hidden=TRUE for test modules? update: this idea is just disaster. don't try at home :)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
4.42 KB

Adding condition to remove system test modules.

vijaycs85’s picture

1. package != Core to path !== core/modules - opens option to have all modules(without hidden or required) under /core/modules. This condition is just to avoid modules that are not under /core/modules.

2. Test modules under system/tests/modulesthat are getting visiblity from system_test_system_info_alter in system_test.module. As this test case is focusing on core modules, skipping these test modules is fine. But not sure this would handle all scenarios/ right approach.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

@vijaycs85: is "Testing" the only package in core that should be ignored outright in this test? Then an exception can be added for package != Testing, and this would not be a problem anymore. Since we seem to want to keep introducing more packages in core, but ignore the test package, this seems like a good solution to me.

vijaycs85’s picture

@Gábor Hojtsy, if our intention is to check just core modules, isn't safe to just check core and multilingual packages?

vijaycs85’s picture

Just in case, if we want to go with #25, here is the patch...

YesCT’s picture

if our intention is to check just core modules, isn't safe to just check core and multilingual packages?

@vijaycs85 Yes.. but No. Because right now, that will work, but those non-multilingual packages *might* be being split out into their own packages also. and if they are, we would have to come back here and update this fix. So if we only ignore testing, we are making a wager that that will be less work for us in the future since we wont have to keep adding exceptions to future possible core packages.

vijaycs85’s picture

Status: Needs work » Needs review

OK, it makes sense. My concerns are
1) Already got two package of modules (Development, Other) which wasn't part of this test - might not necessary for this test and it might increase the simpletest running time (currently it says "2000 passes").
2) If any other package/modules that we would need to skip in future.

I'm *not* suggesting/opposing any here. this is just impact of keeping this test case this way.

sun’s picture

Sorry, I wasn't aware of #24 and that EnableDisableTest extends ModuleTestBase, which enables system_test.module by default, and system_test_system_info_alter() un-hides a couple of testing modules.

There are two ways to work around this:

A) The patch in #23, which additionally excludes the Testing package.

B) Remove the $modules declaration from ModuleTestBase, and instead, add it to the individual test classes that actually need it. AFAICS, EnableDisableTest and RequiredTest do not need system_test.module.

Off-hand, I think that B) is the proper way to go, since base test classes should normally provide test helper methods only, but should not enable modules by default that are doing unholy things (like system_test.module is doing).

YesCT’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/EnableDisableTest.phpundefined
@@ -23,11 +23,15 @@ public static function getInfo() {
-      if ($module->info['package'] != 'Core' || !empty($module->info['hidden']) || !empty($module->info['required'])) {

I have a question:
why not use isset instead of !empty. isset reads more naturally.

I googled a bit ( http://techtalk.virendrachandak.com/php-isset-vs-empty-vs-is_null/ ) but it seems in this case isset would be ok?

YesCT’s picture

Status: Needs review » Needs work

needs work based on #30

vijaycs85’s picture

patch in #27 has 30.A. Wondering if we can take 30.B as a separate issue and fix site wide?

vijaycs85’s picture

Fix for #31 and condition split as it exceeds 80 characters.

YesCT’s picture

Status: Needs review » Needs work

I dont think you need to split the condition because it is more than 80 chars. It's just comments that have to be 80 chars or less.

If you leave it split like that though, a different variable name is needed. !$required_test seems more awkward.

Before
$in_core_path
and
$in_testing_package
were really nice for code readability.

I'd say to leave them in.

sun’s picture

Status: Needs work » Needs review
FileSize
6.66 KB

Attached patch moves the $modules declaration to the tests that require it. Let's see how it goes.

Status: Needs review » Needs work

The last submitted patch, drupal8.module-test.36.patch, failed testing.

Gábor Hojtsy’s picture

@vijaycs85: where in core are modules put into the 'development' or 'other' packages? Btw @sun's test rework idea indeed looks more correct, it does work (yet) though.

vijaycs85’s picture

system_test.module (line no:192)

      drupal_set_message(t('hook_modules_installed fired for @module', array('@module' => $module)));

Removing this assert as there is no system_test.module any more.

vijaycs85’s picture

Status: Needs work » Needs review
Berdir’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/EnableDisableTest.phpundefined
@@ -102,8 +104,6 @@ function testEnableDisable() {
         // the logs.
         foreach ($modules_to_enable as $module_to_enable) {
-          $this->assertText(t('hook_modules_installed fired for @module', array('@module' => $module_to_enable)));
-          $this->assertText(t('hook_modules_enabled fired for @module', array('@module' => $module_to_enable)));
           $this->assertModules(array($module_to_enable), TRUE);

I think these assertions are exactly the reason system_test.module is supposed to be enabled in this test, so neither it nor these assertions should be removed.

system_test_system_info_alter() is already partially wrapped in a state() check. I think we should just add another one for the code below so that it is only activated for that specific test that wants it altered.

Status: Needs review » Needs work

The last submitted patch, drupal8.module-test.39.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
4.69 KB

As per #41, we need to have system_test.module for EnableDisableTest. So just keeping #27 with the recommendation in #31. Thanks in advance for further suggestion.

YesCT’s picture

Little fix for comment standards regarding optional params http://drupal.org/node/1354#doxygen-general

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

@vijaycs85: a *very* similar but smaller issue is at #1879732: Language-related modules not listed on available updates page if you are looking for something else to take on :)

vijaycs85’s picture

Thank you Gábor, I will check it.

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks! Removing sprint tag.

sun’s picture

Assigned: vijaycs85 » Unassigned
Status: Fixed » Needs review
FileSize
897 bytes

Quick follow-up patch to fix a minor regression / API misinterpretation.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yes, we it would be weird to not support setting hidden = FALSE and required = FALSE even though no one will probably ever do that.

YesCT’s picture

I was wondering about that in #31

tstoeckler’s picture

Right, I totally agreed with you in #31 at the time, I didn't think of the use-case in #51 either.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Probably worth a quick test, no?

Gábor Hojtsy’s picture

@webchick: to have a module that satisfies the condition fixed, it would need to be:

- in the code core module's path
- not be a test module

(these two are the conditions not changed), BUT

- be a module with an .info file specifically providing hidden = FALSE OR
- be a module with an .info file specifically providing required = FALSE

So that means we would need to change one of the real core module's .info files with superfluous .info file properties.

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

Right. We would have to add hidden = FALSE to a real core module. That wouldn't actually hurt anyone, but it would be a huge WTF, and I don't think we want to do that. Moving back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ah, ok. Fair enough.

Committed and pushed to 8.x. Thanks!

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