Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
I can manually verify a bug by installing Drupal with the Minimal profile and then visiting the modules page, but doing exactly the same thing via Simpletest does not show the error. See http://drupal.org/node/1170362#comment-5794850 for code and screenshots.
Proposed resolution
Remaining tasks
Figure out what Simpletest is doing differently and/or how to mimic the manual steps via Simpletest.
User interface changes
API changes
Original report by pwolanin
Per http://drupal.org/node/279455
Right now drupal_web_test_case always uses the default profile when installing. This is problematic for a couple reasons - for example, I can't test an alternate profile to make sure it runs correctly.
Comment | File | Size | Author |
---|---|---|---|
#39 | 279515-39.variable-profile.patch | 9.75 KB | dww |
#39 | 279515-39.variable-profile.interdiff.txt | 1000 bytes | dww |
#37 | 279515-variable-profile.patch | 9.75 KB | boombatower |
#36 | 279515-variable-profile.patch | 10.47 KB | boombatower |
#33 | 279515-variable-profile.patch | 10.46 KB | boombatower |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedHere's essentially the same patch for 7.x as #2 on that issue.
Comment #2
pwolanin CreditAttribution: pwolanin commentedSo, I think this would now allow you to test different profiles using code like:
Comment #3
boombatower CreditAttribution: boombatower commentedI think it should be simpler and more documented.
Possible
and on tearDown the class variable
$install_profile
could be reset.Comment #4
pwolanin CreditAttribution: pwolanin commentedhow about the attached?
One doesn't even need to reset the variable, I think, since this will be fresh for each class that's instantiated.
Comment #5
catchI like the function name. Useful functionality to have. Not tested yet.
Comment #6
boombatower CreditAttribution: boombatower commentedI like this implementation much better although I think in needs to be reset in
tearDown
so that the next test will always use the default profile.Comment #7
pwolanin CreditAttribution: pwolanin commentedpatch with bonus test.
I still don't think the addition to tearDown is really needed, but it doesn't hurt.
Comment #8
Dries CreditAttribution: Dries commentedPersonally, I think
setInstallProfile
is behaving a little funny.1. The return value can be 3 different things. Because this is a class, we don't necessarily have to do the funny return value thing for
setInstallProfile
.2. To reset the install profile, why don't we set it to 'default' using
$this->setInstallProfile('default')
instead of$this->setInstallProfile(NULL, TRUE)
? It seems like reset is not needed?Comment #9
pwolanin CreditAttribution: pwolanin commentedright, we could also set a class variable rather than the internal static - cleaned up version attached.
Comment #10
cwgordon7 CreditAttribution: cwgordon7 commentedWhy the line:
"+ $this->test_profile = variable_get('web_test_case_profile', 'default');"
?
Comment #11
pwolanin CreditAttribution: pwolanin commentedBecause those building a new or complicated profile might want to run ALL tests with that profile as the install and/or a packaged distribution of Drupal might not include the default profile.
Comment #12
pwolanin CreditAttribution: pwolanin commentedfrom IRC conversion, I misunderstood the cwgordon's concern - the code was unclear (and only worked by luck) - it's clearer and more reliable to move the determination of the profile to prior to where the DB prefix is altered.
Comment #13
dropcube CreditAttribution: dropcube commentedCurrently, there are a lot of assertions relying on the HTML response. For example,
drupalLogout
checks if the user name field is found when requesting the /user page. If we are going to test alternate profiles that may install multiple modules we should have in mind that such modules may alter the output in any way. For example, consider a module that alters or replace the user login form, then every test that usesdrupalLogout
will fail due to the absence of the name field in whichdrupalLogout
bases it's assertions.So, here I see again the real need of having data available to make assertions, and not basing assertions 100% on the HTML response. See #266220: Exposing metadata to tests
Comment #14
pwolanin CreditAttribution: pwolanin commented@dropcube - yes, that's certainly true - but I'm not sure there is any ready way around that. This patch just enables the use of other profiles - so if you do, obviously you'll have to take that into consideration when interpreting test results.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commented@dropcube: I already expressed my concerns about #266220: Exposing metadata to tests. If I understand pwolanin approach here, we want to test profiles themselves (ie. have a default.test and a expert.test), not use existing tests on different profile configurations.
Again: we are testing Drupal in its standard configuration. We cannot even imagine testing every possible configuration out there.
Comment #16
pwolanin CreditAttribution: pwolanin commented@Damien Tournoud - one motivation is to allow you to test profiles - but this patch enables a number of possibilities
Comment #17
dropcube CreditAttribution: dropcube commentedThe .test file attached uses a more generic approach by implementing a class
InstallProfileTestCase
which other profile tests may extend. This class contains common functions, for example,_testInstalledModules()
. Also a test for the default profile has been added.Comment #18
dropcube CreditAttribution: dropcube commentedHere is the patch based on pwolanin's #12 with the approach described at #17
Comment #19
pwolanin CreditAttribution: pwolanin commentedIf InstallProfileTestCase is supposed to be a generic base class to be extended, it should not have any tests in it.
Comment #20
dropcube CreditAttribution: dropcube commentedHere is the updated patch. The base class does not contain test functions now, but assertions.
Comment #21
dropcube CreditAttribution: dropcube commentedInstall profiles Tests: 25 passes, 0 fails, 0 exceptions
Comment #22
pwolanin CreditAttribution: pwolanin commentedsince drupal_verify_profile() does some extra checking and might eliminate some modules from the list in the profile, I'm not sure we want to use it for the test. I'd suggest reproducing the first part of the code from that function.
Comment #23
dropcube CreditAttribution: dropcube commentedI am attaching an updated version of the file install.test for review and not a patch because seems to be a conflict in drupal_web_test_case.php due to #231190: Page Cache doesn't work with HEAD requests
In the test now are implemented #22 pwolanin's suggestions and added other tests for the default profile. Will submit a patch later.
Comment #24
dropcube CreditAttribution: dropcube commentedHere is an updated patch against HEAD.
Comment #25
pwolanin CreditAttribution: pwolanin commentedThis looks pretty good - made a few tweaks. For example, not every test needs to check that the invalid profile name is rejected.
Comment #26
moshe weitzman CreditAttribution: moshe weitzman commentedsubscribe. a worthwhile patch. not time to test now though.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #28
webchickI'd still really like to see this functionality added. I'm trying to write tests for Project Issue module, and it'd be much, much easier if I could re-image durpalorg_testing profile on each testX function, rather than default.
SimpleTest module improvements are still allowed as long as they don't break APIs.
Comment #29
dwwwebchick: +1 to that. ;) We're working on Project* tests for d.o unit testing (d.o upgrades, redesign deployment, etc). We need the same thing. Maybe one of us will get this working today.
Comment #30
hunmonk CreditAttribution: hunmonk commentedupdated patch. same functionality as the last patch, but a lot has changed in the code since then -- hopefully i got things right. let's see what the test bot says...
Comment #31
hunmonk CreditAttribution: hunmonk commentedbah, forgot to include the install.test file. reroll with that added.
Comment #32
cwgordon7 CreditAttribution: cwgordon7 commentedThis looks fundamentally good. There's some minor coding style stuff (trailing whitespace, simpletest stuff, etc.) that I'd like to fix up later tonight, but other than that this should be just about rtbc.
Comment #33
boombatower CreditAttribution: boombatower commentedTests for includes are placed in simpletest/tests since all test must be in a module. I moved the install.test test cases to the profiles they relate to and put the base test case in standard.test. This was a lot of fun since I needed to get the registry to parse all install profiles instead of just the active one.
I do not think we want to revert to the standard profile without at least adding a failed assertion or some such, although I am not sure if we want to check it directly or not. With all the other class variables we stay away form get'ers and set'ers so probably keeep with that pattern when working with install profiles.
Since the tests will be closely related to the install profile used a general suite wide variable seems like a bad idea. Tests either want a custom install profile or they don't.
Missing public static on getInfo() and scope modifiers (protected) on other methods. We also no longer put the Implements docblock on setUp() and getInfo(). Few other minor things like t()'s and what not (try to move away form them).
- Test case names should match profile namespace.
- Number of documentation issues.
- Tests don't run since functions are called that do not exists such as hook_profile_modules().
- A number of areas seem to be using outdated API style.
- A bunch of notices in standard.test.
It would appear this was never tested since you cannot run test tests when placed in includes/ and even when placed somewhere will they will run they blow up. After about 3 hours I got everything working, but it was far from done.
Comment #34
boombatower CreditAttribution: boombatower commentedActually so many changes this could use another review.
Comment #35
boombatower CreditAttribution: boombatower commentedThe way I wrote the patch it will allow the registry to find profiles in places outside of ./profiles which to me seems like something the system should do. So if I get agreement I can file a followup patch that finds the other places in core that hard-code a reference to it in ./profiles.
Comment #36
boombatower CreditAttribution: boombatower commentedLaptop isn't configured for spaces properly, fixed patch to remove tabs.
Comment #37
boombatower CreditAttribution: boombatower commentedThis should fix the number of tests run.
Comment #38
dwwThis patch is looking very close. I only saw two minor problems:
A)
B)
The test function names don't match the classes. Trivial re-roll, 1 sec.
Comment #39
dwwLike so. I just edited the patch file directly in this case. ;)
Comment #40
boombatower CreditAttribution: boombatower commentedYep, looks good. I changed the class names to match, but forgot the test methods. Do we need another review or should we RTBC this?
Comment #41
dmitrig01 CreditAttribution: dmitrig01 commentedRead the patch, tried it. Looks good.
This has the side affect that if you're really evil you can enable two install profiles. Muahahaha. But this is awesome. Something that should be exposed via UI in Drupal 8...
Comment #42
boombatower CreditAttribution: boombatower commentedUI? The tests are designed to work with one profile, not multiple.
Comment #43
catchIt would be useful for some modules, for example entitycache, to be able to run all core tests as if they were part of the default profile, since the objective there is to not break core, and core (still) doesn't reliably use it's own API functions for updating entities in all cases. However that's a different issue to this patch.
Comment #44
jhedstromSubscribing. @boombatower, have you already started a back-port of this feature to 6.x? If not, I can probably throw one together.
Comment #45
boombatower CreditAttribution: boombatower commentedNot at the moment...life has been ultra crazy lately.
Comment #46
catch#39: 279515-39.variable-profile.patch queued for re-testing.
Comment #48
rfaysubscribe
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedme too.
not sure about variable_set/variable_get here, looking into it.
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedbeing able to set the install profile at the class level is too coarse a scope for this issue: #323477: Increase simpletest speed by running on a simplified profile.
for that, we need to be able to set the profile per test, so we'd need something like:
with a default, so only tests that don't want to use the default need define it.
Comment #51
sunBetter title. #323477: Increase simpletest speed by running on a simplified profile already focuses on allowing a certain install profile for running tests. This issue seems to be about testing install profile expectations itself.
Comment #52
sunClosely related: #1215104: Use the non-interactive installer in WebTestBase::setUp()
Comment #53
sunComment #54
pillarsdotnet CreditAttribution: pillarsdotnet commentedThis blocks writing a test for #1170362-145: Install profile is disabled for lots of different reasons and core doesn't allow for that
Comment #55
sunI've reread this entire issue once more and looked at the most recent but dated patches. I do not see any new or changed functionality that would not be possible already.
#1373142: Use the Testing profile. Speed up testbot by 50% changed the default profile for running tests to the Testing profile. Individual test cases can still override that decision to use the Standard, Minimal, or whatever else profile being available for running tests by assigning
protected $profile = 'standard';
Additionally, #911354: Tests in profiles/[name]/modules cannot be run and cannot use a different profile for running tests fixed the discovery of test cases provided by the installation profile or modules in an installation profile when the installation profile of the test is different to the installation profile of the site that executes the test (i.e., the parent site).
Thus, I don't understand what this issue is about. If I'm mistaken, then please do not clarify in a comment, but revise/revamp the issue summary instead, as it doesn't hold any information at all currently.
Comment #56
pillarsdotnet CreditAttribution: pillarsdotnet commentedUpdated summary as requested.
Comment #57
sunThere's no relationship between that issue and this issue, other than that you're dealing with a strange, installation profile related testing system oddity and this issue happens to have that term in its title.
#1215104: Use the non-interactive installer in WebTestBase::setUp() is way more related, as you seem to experience a difference in installation profile behavior between manual testing and automated testing.
Looked once more at the patch of this issue today, and confirming once again, all of the changes exist in the current codebase already - either literally or through similar means. So let's close this issue to prevent further confusion.
Comment #58
pwolanin CreditAttribution: pwolanin commentedI don't see how the linked issue duplicates this? Certainly for 7 I cannot yet set an install profile to use instead of standard?
Comment #59
sunD7 also supports
protected $profile = 'standard';
already.Can you clarify what is not working?
Comment #60
pfrenssenSure this is working in D7, for an example, see SimpleTestOtherInstallationProfileModuleTestsTestCase().
Comment #61
joachim CreditAttribution: joachim commented> D7 also supports protected $profile = 'standard'; already.
It does.
But that does not cause the install profile to be used. It causes the modules listed in that install profile's .info file to be loaded.
The install code in the profile is not run. See #2038823: DrupalWebTestCase::setUp() does not run hook_install_tasks() for the selected profile.
Comment #62
pfrenssenAre these issues duplicating eachother? If so, can you mark one of them as a duplicate?
Comment #63
joachim CreditAttribution: joachim commentedClosed the other one as a duplicate, and changing this to a bug, making title more specific.
Comment #63.0
joachim CreditAttribution: joachim commentedUpdated summary per sun's request in #55.