Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem
- #1833184: Find a consistent naming scheme for translation-related modules moved all language-related modules from "Core" into a new "Multilingual" package.
- EnableDisableTest only tests enabling and disabling of all Core modules.
Details
- EnableDisableTest must not test any non-core modules that may exist.
Proposed solution
- Change EnableDisableTest to retrieve all available modules and filter them based on their filesystem path instead of the package name.
Comment | File | Size | Author |
---|---|---|---|
#50 | drupal8.module-test.50.patch | 897 bytes | sun |
#44 | 1871328-44-test-case-change.patch | 4.73 KB | YesCT |
#44 | interdiff-43-44.txt | 786 bytes | YesCT |
#43 | 1871328-43-test-case-change.patch | 4.69 KB | vijaycs85 |
#39 | drupal8.module-test.39.patch | 7.27 KB | vijaycs85 |
Comments
Comment #1
Gábor Hojtsy"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.
Comment #2
YesCT CreditAttribution: YesCT commentedI 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".
Comment #3
vijaycs85Giving a try.
Comment #5
vijaycs85Comment #6
vijaycs85Sorry for that TDD approach. updating test cases to get package dynamically.
Comment #7
tstoecklerIt seems this is missing the check on the filepath, no?
Comment #8
vijaycs85oops...good catch tstoeckler
Comment #9
tstoecklerI 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.
Comment #10
Gábor Hojtsy@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.
Comment #11
Gábor HojtsyComment #12
Gábor HojtsyOk, that assumption does not seem to be true :)
Comment #13
vijaycs85Not 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 :(
Comment #14
Gábor HojtsyLooks 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.
Comment #15
sunLet'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:
Comment #16
vijaycs85For 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.
Comment #17
vijaycs85cross post... updating fix in #15
Comment #19
vijaycs85Though strpos() does something funny, all these hidden modules should caught by 2nd part of the condition ...
Comment #20
YesCT CreditAttribution: YesCT commentedI used devel, and looked.
Here requirements1_test is in the $modules array.
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...
Comment #21
YesCT CreditAttribution: YesCT commentedI cannot find this assert message:
I searched for "that can be enabled".
The first one I see is from:
Comment #22
vijaycs85YesCT, 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 :)
Comment #23
vijaycs85Adding condition to remove system test modules.
Comment #24
vijaycs851.
package != Core
topath !== 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
insystem_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.Comment #25
Gábor Hojtsy@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.
Comment #26
vijaycs85@Gábor Hojtsy, if our intention is to check just core modules, isn't safe to just check core and multilingual packages?
Comment #27
vijaycs85Just in case, if we want to go with #25, here is the patch...
Comment #28
YesCT CreditAttribution: YesCT commented@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.
Comment #29
vijaycs85OK, 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.
Comment #30
sunSorry, 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).
Comment #31
YesCT CreditAttribution: YesCT commentedI 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?
Comment #32
YesCT CreditAttribution: YesCT commentedneeds work based on #30
Comment #33
vijaycs85patch in #27 has 30.A. Wondering if we can take 30.B as a separate issue and fix site wide?
Comment #34
vijaycs85Fix for #31 and condition split as it exceeds 80 characters.
Comment #35
YesCT CreditAttribution: YesCT commentedI 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.
Comment #36
sunAttached patch moves the $modules declaration to the tests that require it. Let's see how it goes.
Comment #38
Gábor Hojtsy@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.
Comment #39
vijaycs85system_test.module (line no:192)
Removing this assert as there is no system_test.module any more.
Comment #40
vijaycs85Comment #41
BerdirI 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.
Comment #43
vijaycs85As 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.
Comment #44
YesCT CreditAttribution: YesCT commentedLittle fix for comment standards regarding optional params http://drupal.org/node/1354#doxygen-general
Comment #45
Gábor HojtsyLooks good to me!
Comment #46
webchickCommitted and pushed to 8.x. Thanks!
Comment #47
Gábor Hojtsy@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 :)
Comment #48
vijaycs85Thank you Gábor, I will check it.
Comment #49
Gábor HojtsyThanks! Removing sprint tag.
Comment #50
sunQuick follow-up patch to fix a minor regression / API misinterpretation.
Comment #51
tstoecklerYes, we it would be weird to not support setting
hidden = FALSE
andrequired = FALSE
even though no one will probably ever do that.Comment #52
YesCT CreditAttribution: YesCT commentedI was wondering about that in #31
Comment #53
tstoecklerRight, I totally agreed with you in #31 at the time, I didn't think of the use-case in #51 either.
Comment #54
webchickProbably worth a quick test, no?
Comment #55
Gábor Hojtsy@webchick: to have a module that satisfies the condition fixed, it would need to be:
- in the
codecore 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.
Comment #56
tstoecklerRight. 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.Comment #57
webchickAh, ok. Fair enough.
Committed and pushed to 8.x. Thanks!