Move includes/tests to simpletest/tests & provide hidden .info propery

Just ran all tests with the patch applied, exactly the same results as without.

side note - is there an issue for this attachment thing, had the same issue yesterday.

This patch would be easier to evaluate if one of the core tests utilized this functionality. I suggest the XML-RPC tests, since they currently fail to run entirely because simpletest_xmlrpc.module doesn't exist.

That would make the third test fixed by that patch...hmm...now we just need Dries :)

#260778: Add SimpleTest user agent information to drupal_http_request is committed so this needs to be re-evaluated.

Currently working on this.

There were numerous issues with the original patches which didn't run at all. Rather than list them all just diff the patches if your interested.

I have included a patch the allows tests to implement module hooks using a getInfo() argument.

A second patch reactors xmlrpc.test to use the new API.

Test modules are defined as such:

'test_modules' => array('xmlrpc_validator1_test')

I'm not sure if there are any other caches that need clearing after the test modules are enabled, but it works for the one implementation case we have.

The xmlrpc.test could use a bit of a cleanup to make it easier to read, but that is for another issue.

You will need to apply #245961: XMLRPC value_calculate_type throws exception to get no exceptions.

Ok, I tested patch in #7, and the XML-RPC tests pass, now!

There are only two exceptions that have been identified (and solved) in #245961.

I validated that if you mess with one of the tested function (for example adding a random value to the result of xmlrpc_validator1_test_arrayOfStructsTest()) both the test of that function and the multicall test fail.

The test itself needs cleanup (test descriptions have spurious %s and lack proper capitalization), but this can go in another issue.

Hmm. It might might make more sense to let the registry scan test files. This would be 1 line patch to registry.inc and a bunch of lines in *.info. Perhaps registry could refuse to recognize these implementations if we aren't running a test. I'd like for chx to have a look here before we commit.

drupal_alter('system_info', $files[$filename]->info, $files[$filename]); we have this in module_rebuild_cache. If this would move before the if (empty($file->info)) then we could a) pre-seed the sytem table with the fake modules -- put the code before drupal_install_modules in setUp b) alter the nonexisting .info file as necessary. This makes it possible that even if someone calls something that in turn calls module_rebuild_cache then our fake modules won't disappear. And, we do not need to reset all those caches, implant stuff into module_list...

@chx: good suggestion, but we can't really do that because at the time drupal_alter('system_info') is called there are no enabled modules...

at that time there are because before the install procedure you have the modules of the 'mother drupal' installed . i guess there are all sorts of problems with my approach so it might not be the best -- namely , how do you know when to alter and when not to? can become ugly quick. So, back to CNR.

@chx: moreover only real "*.module" files are considered by module_rebuild_cache(), so this is a dead-end. But I do aggree that these 'fake' modules should look the same as real ones.

An alternative approach: create a real .module and its associated .info and define a special .info parameter that hides the module from the administrative page.

The only issue with the real .module and .info would be size. Ideally I think it would be nice to place all the fake modules in the .test file, but that would alleviate the issue that I had to get around which is the test class being loaded in the test environment which doesn't have SimpleTest loaded and crashes. That is why the following code wraps the test class.

// Make sure we are not running in simpletest call.
if (!preg_match("/^simpletest\d+/", $GLOBALS['db_prefix'])) {
...class...
}

Just to make sure we all agree, the test needs cleanup, but that is a separate patch that I would be happy to do once this gets in.

Talked with webchick, DamZ, and chx in IRC and came up with the following.

• Move /include tests to SimpleTest module folder so they can have test modules.
• Add /tests directory back to modules.
• Allow for hidden property in .info files.
• Implement new API in xmlrpc.test

I've started working on this.

Ok the above has been implemented and all seems to work well.

Applies perfectly and work as intended.

The only thing I could find is a typo in the module description (my bad, sorry). It should be:

description = "Support module for XML-RPC tests according to the validator1 specification."

And sorry again, XMLRPC should be XML-RPC in the test module name.

Otherwise, great job!

grr.

Updated patch. Remember to still apply system_hidden_property.patch from #24.

What a great trick. I like it.

This is indeed the least hackish way to allow our tests to test hook implementations -- put them in a module themselves. Over the past weeks, we've tried all kinds of weird tricks, including declaring functions at run-time, manipulating the registry data, etc. In the end, this makes the most sense, and will also test the "normal" module execution path, which is what we care about -- will this hook, if added to a "real life" module, actually succeed?

As an added bonus, the hidden = true property in .info files can be used on Drupal sites that have one or two custom modules that absolutely CANNOT be disabled or they completely break the site functionality. Currently, you have to do some hack like putting DO NOT DISABLE THIS in the .info file's description. This would allow you to enforce that in a much better way. It'd also be handy for hosted Drupal platforms to hide "internal" modules such as database backup modules and so on.

So yeah. +1.

Great patch indeed.

Can we get this committed? It's a blocker for other tests that requires modules (for example: #235673: Changes to block caching mode not caught, which is itself a Views2 blocker).

I hope it get's committed, cause I also want to test some hook functions.

Aside from the new xmlrpc_test.module, it looks like #27 is just a patch to do a history losing CVS rename. Not sure we care much about the revision history in these test files, but if so, we could pull out cvs_rename.pl for this. See #218973: Reorganize CCK in CVS and #254655: Run CVS move script for OG HEAD for some examples. It'd be pretty trivial.

FYI, I want to think a bit more about the (ab)use of .info files before committing this patch. I'll get to that shortly.

After re-reading this issue, I'm still not 100% about the use case for this patch. Why would tests want to implement hooks and pretend they are modules?

I can see a use case for the XML-RPC tests but it looks like that could be solved differently. Any other use cases? Sorry for asking questions but I'd like to get the complete picture.

@Dries: The use cases here are all tests of core features that are not used anywhere in core, or only used partially (not covering the whole test cases we want to test).

XML-RPC is really archetypal for that, because it has no feature of its own, and we have to rely on other modules to test it.

In #235673: Changes to block caching mode not caught, I also implemented a test for block caching using the same technique. Of course there are core modules that use block caching, but none of them change their caching mode during their lifecycle.

We seems to be in the trend of removing more and more modules from core (Poll and Blogs are candidate), so the possibility to test core features that are not used in core will become more and more useful.

Dries, any time we need a whole page -- requires a hook_menu entry. If we want to check whether hook_foo operates properly we need to implement hook_foo. And so on. We need test modules, no doubt.

A great use-case for the hidden attribute aside from SimpleTest is given in my Usability Testing Suite.

The Proctor module should only be enabled automatically in the test environment. I have no way of preventing the user setting up the test from enabling the module and getting weird behavior. The best I can do in 6.x without this patch (which probably won't be back-ported anyway) is put the following in the .info file.

description = "<b>DO NOT ENABLE.</b> Provide interface for testers while running in the test environment."

@Dries: The DB TNG effort includes unit tests that absolutely require hook_schema and hook_query_alter implementations. Right now those are implemented in a dedicated dummy module. Whether we handle this as "real" modules or as "fake" modules, this is a clear use case where test-specific hook implementations are required.

I have not reviewed this patch itself to know if it's any good or not, but the need for test-only hook implementations is real.

The idea of "fake" modules can also be extended to "fake" themes, which can be used to test the theme layer. The idea of having 'hidden' themes may be useful as well ?

@dropcube: for now, there is no need for fake themes, and this is not the scope of this patch. As far as I know, lots of core theme functions are overridden either in phptemplate or in garland, so we are probably covered here.

Ping

I'm not cool with moving the tests to "modules/simpletest/tests". It is a lot better to put the tests closer to the code and to have a predictable pattern as to where tests can be found. It's not not natural to go look for "modules/simpletest/tests/common.test" if you are looking at "common.inc" in the includes directory.

Why don't we put the required glue code in simpletest.module? Simpletest.module could define the XML-RPC callbacks required for the tests to run.

Why don't we put the required glue code in simpletest.module? Simpletest.module could define the XML-RPC callbacks required for the tests to run.

Unfortunately, that falls apart when we want to test hook_menu() or something that SimpleTest module already implements. It also means I can't test the hooks in isolation.

I was thinking less modules/simpletest/tests/common.test and more like modules/simpletest/tests/core_hooks.test, modules/simpletest/tests/node_hooks.test, etc.

Having simpletest.module implement hooks for testing won't work as currently setup since SimpleTest isn't enabled in testing environment, and if it was it would deviate from the default Drupal install (not a good idea). Secondly it doesn't allow you to test any hook you want and in any way you want, like webchick said.

Moving the tests out of include is to allow the include tests to provide test modules to implement hooks. It was thought that it would make more sense to have the tests modules and the code that uses them in the same folder. Since the module system doesn't and shouldn't scan the includes directory it wouldn't pick up test modules.

Instead the include tests could be left where they are and could just define test modules in simpletest/tests.

I am asking Dries to reconsider... Edit: at least the hidden module property.

http://drupal.org/node/276430 is a poster child for hidden modules. The easiest to check for persistence (and most probably the only hack free way) is to have a page callback which sets something in SESSION and then another that dumps it.

I've spent a large portion of my evening thinking about alternative solutions and I've come to the conclusion that this patch is probably the best way forward. I'll think about it some more tomorrow on the plane, but I recommend that you reroll this patch so that it applies. Thanks for feeding me with all the tidbits of information -- that helped.

If we move xmlrpc_test.module, it might make sense to rename it from 'tests/xmlrpc_test.module' to 'test/xmlrpc.module'. The _test seems pretty redundant now, as we already have 'test' in in the directory name. That would clean it up a little ...

I would prefer to keep the _test suffix at the end of test module names, in order to have a consistent naming scheme and to avoid potential clashes between module names.

This way we will have:

• node_test.module for node hook tests
• block_test.module for block hook tests
• and thus xmlrpc_test.module for XML-RPC tests

Working on this.

@cwgordon7: the patch in #27 dropped the module_exists('simpletest_xmlrpc') check, which is not needed any more.

I believe this should do the trick.

system_hidden_property.patch - removes modules from the list if they have the hidden property.
simpletest_move_includes_tests.patch - turns of the checking for tests in includes/tests, creates the fixed xml_rpc.test and xmlrpc_test.module.

I was assuming we are moving tests from includes/tests. If that is incorrect then I can re-roll, but this patch doesn't move the other 7 tests from includes/tests as that is just patch bloat.

Leaving _test suffix in name as #50 was the original reason which I think still stands.

Note: System patch comes with some trailing whitespace cleared.

I agree with leaving the _test suffix just for clarity that they aren't submodules of simpletest or whatever. Both patches apply cleanly, xmlrpc test actually runs and passes. So back to RTBC.

I've committed this to CVS HEAD. Because I'm almost out of battery, I could not re-run the tests. Hopefully I got it right, and hopefully we can start making rapid progress again with testing. Yay. :-)

Marking fixed :)

The patch

turns of the checking for tests in includes/tests

and

I was assuming we are moving tests from includes/tests. If that is incorrect then I can re-roll, but this patch doesn't move the other 7 tests from includes/tests as that is just patch bloat.

That being said the tests in includes/tests are not being recognized by SimpleTest and we have two xmlrpc.test files the one in simpletest/tests being correct.

The tests in includes/tests either need to be moved to simpletest/tests or we need to add the code in back to SimpleTest to scan includes/tests again.

I think for consistency the tests should move to simpletest/tests since the test modules have to be in the modules folder, but that isn't required if we add the code back.

The plan approved in #54 was to move all test files from includes/tests to modules/simpletest/tests. This still has to be done, because tests from includes/tests are not recognized anymore.

Do we have to request something from the infrastructure team to do this properly?

If we want to keep the revision history for the files, otherwise Dries can just move them.

Moved the tests. Committed to CVS HEAD. Thanks.