Posted by scor on April 22, 2010 at 11:08pm
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | simpletest.module |
| Category: | task |
| Priority: | normal |
| Assigned: | boombatower |
| Status: | needs work |
| Issue tags: | Testing system |
Issue Summary
As discussed with boombatower, .test files should not live in the tests directories:
./modules/field/modules/list/tests/list.test
./modules/field/tests/field.test
./modules/file/tests/file.test
./modules/simpletest/tests/actions.test
./modules/simpletest/tests/ajax.test
./modules/simpletest/tests/batch.test
./modules/simpletest/tests/bootstrap.test
./modules/simpletest/tests/cache.test
./modules/simpletest/tests/common.test
./modules/simpletest/tests/database_test.test
./modules/simpletest/tests/error.test
./modules/simpletest/tests/file.test
./modules/simpletest/tests/filetransfer.test
./modules/simpletest/tests/form.test
./modules/simpletest/tests/graph.test
./modules/simpletest/tests/image.test
./modules/simpletest/tests/lock.test
./modules/simpletest/tests/mail.test
./modules/simpletest/tests/menu.test
./modules/simpletest/tests/module.test
./modules/simpletest/tests/path.test
./modules/simpletest/tests/registry.test
./modules/simpletest/tests/schema.test
./modules/simpletest/tests/session.test
./modules/simpletest/tests/theme.test
./modules/simpletest/tests/unicode.test
./modules/simpletest/tests/update.test
./modules/simpletest/tests/xmlrpc.testgit is your friend if you want to avoid messing with CVS.
Comments
#1
All tests in core, based on
find . -name "*.test"../modules/aggregator/aggregator.test./modules/color/color.test
./modules/dblog/dblog.test
./modules/file/tests/file.test
./modules/locale/locale.test
./modules/system/system.test
./modules/translation/translation.test
./modules/node/node.test
./modules/tracker/tracker.test
./modules/simpletest/simpletest.test
./modules/simpletest/tests/path.test
./modules/simpletest/tests/mail.test
./modules/simpletest/tests/module.test
./modules/simpletest/tests/error.test
./modules/simpletest/tests/file.test
./modules/simpletest/tests/common.test
./modules/simpletest/tests/graph.test
./modules/simpletest/tests/update.test
./modules/simpletest/tests/lock.test
./modules/simpletest/tests/theme.test
./modules/simpletest/tests/filetransfer.test
./modules/simpletest/tests/schema.test
./modules/simpletest/tests/ajax.test
./modules/simpletest/tests/session.test
./modules/simpletest/tests/bootstrap.test
./modules/simpletest/tests/registry.test
./modules/simpletest/tests/database_test.test
./modules/simpletest/tests/unicode.test
./modules/simpletest/tests/menu.test
./modules/simpletest/tests/image.test
./modules/simpletest/tests/cache.test
./modules/simpletest/tests/actions.test
./modules/simpletest/tests/form.test
./modules/simpletest/tests/xmlrpc.test
./modules/simpletest/tests/batch.test
./modules/blog/blog.test
./modules/forum/forum.test
./modules/user/user.test
./modules/help/help.test
./modules/statistics/statistics.test
./modules/image/image.test
./modules/poll/poll.test
./modules/update/update.test
./modules/comment/comment.test
./modules/menu/menu.test
./modules/filter/filter.test
./modules/openid/openid.test
./modules/syslog/syslog.test
./modules/contact/contact.test
./modules/php/php.test
./modules/field/tests/field.test
./modules/field/modules/options/options.test
./modules/field/modules/field_sql_storage/field_sql_storage.test
./modules/field/modules/list/tests/list.test
./modules/field/modules/text/text.test
./modules/book/book.test
./modules/field_ui/field_ui.test
./modules/trigger/trigger.test
./modules/path/path.test
./modules/block/block.test
./modules/profile/profile.test
./modules/search/search.test
./modules/rdf/rdf.test
./modules/taxonomy/taxonomy.test
./modules/shortcut/shortcut.test
#2
I believe this is the move structure.
./modules/aggregator/aggregator.test./modules/color/color.test
./modules/dblog/dblog.test
-./modules/file/tests/file.test
+./modules/file/file.test
./modules/locale/locale.test
./modules/system/system.test
./modules/translation/translation.test
./modules/node/node.test
./modules/tracker/tracker.test
./modules/simpletest/simpletest.test
-./modules/simpletest/tests/path.test
+./modules/path/path.test
-./modules/simpletest/tests/update.test
+./modules/update/update.test
-./modules/simpletest/tests/mail.test
-./modules/simpletest/tests/module.test
-./modules/simpletest/tests/error.test
-./modules/simpletest/tests/file.test
-./modules/simpletest/tests/common.test
-./modules/simpletest/tests/graph.test
-./modules/simpletest/tests/lock.test
-./modules/simpletest/tests/theme.test
-./modules/simpletest/tests/filetransfer.test
-./modules/simpletest/tests/schema.test
-./modules/simpletest/tests/ajax.test
-./modules/simpletest/tests/session.test
-./modules/simpletest/tests/bootstrap.test
-./modules/simpletest/tests/registry.test
-./modules/simpletest/tests/database_test.test
-./modules/simpletest/tests/unicode.test
-./modules/simpletest/tests/menu.test
-./modules/simpletest/tests/image.test
-./modules/simpletest/tests/cache.test
-./modules/simpletest/tests/actions.test
-./modules/simpletest/tests/form.test
-./modules/simpletest/tests/xmlrpc.test
-./modules/simpletest/tests/batch.test
+./modules/system/tests/mail.test
+./modules/system/tests/module.test
+./modules/system/tests/error.test
+./modules/system/tests/file.test
+./modules/system/tests/common.test
+./modules/system/tests/graph.test
+./modules/system/tests/lock.test
+./modules/system/tests/theme.test
+./modules/system/tests/filetransfer.test
+./modules/system/tests/schema.test
+./modules/system/tests/ajax.test
+./modules/system/tests/session.test
+./modules/system/tests/bootstrap.test
+./modules/system/tests/registry.test
+./modules/system/tests/database_test.test
+./modules/system/tests/unicode.test
+./modules/system/tests/menu.test
+./modules/system/tests/image.test
+./modules/system/tests/cache.test
+./modules/system/tests/actions.test
+./modules/system/tests/form.test
+./modules/system/tests/xmlrpc.test
+./modules/system/tests/batch.test
./modules/blog/blog.test
./modules/forum/forum.test
./modules/user/user.test
./modules/help/help.test
./modules/statistics/statistics.test
./modules/image/image.test
./modules/poll/poll.test
./modules/update/update.test
./modules/comment/comment.test
./modules/menu/menu.test
./modules/filter/filter.test
./modules/openid/openid.test
./modules/syslog/syslog.test
./modules/contact/contact.test
./modules/php/php.test
-./modules/field/tests/field.test
+./modules/field/field.test
./modules/field/modules/options/options.test
./modules/field/modules/field_sql_storage/field_sql_storage.test
-./modules/field/modules/list/tests/list.test
+./modules/field/modules/list/list.test
./modules/field/modules/text/text.test
./modules/book/book.test
./modules/field_ui/field_ui.test
./modules/trigger/trigger.test
./modules/path/path.test
./modules/block/block.test
./modules/profile/profile.test
./modules/search/search.test
./modules/rdf/rdf.test
./modules/taxonomy/taxonomy.test
./modules/shortcut/shortcut.test
#3
Cool that this is decided. Seems like a good move for consistency.
What'd be really nice and would help make this migration easier would be for someone to start from http://drupal.org/files/issues/254655_og_move.25.txt and make a 779204_test_move.txt script that does all the rename operations y'all want to see. That script is actually a lot more complicated reorganization than what we're talking about here. All we really need is a listing of lines like this:
`$cvs_rename modules/field/tests/field.test modules/field/field.test`;...
I'll handle all the setup stuff at the top of the script. That'll be a lot easier for me to test on a copy of the core repo so we can make this change.
Thanks,
-Derek
#4
How about this:
`$cvs_rename ./modules/file/tests/file.test ./modules/file/file.test`;`$cvs_rename ./modules/simpletest/tests/path.test ./modules/path/path.test`;
`$cvs_rename ./modules/simpletest/tests/update.test ./modules/update/update.test`;
`$cvs_rename ./modules/simpletest/tests/mail.test ./modules/system/tests/mail.test`;
`$cvs_rename ./modules/simpletest/tests/module.test ./modules/system/tests/module.test`;
`$cvs_rename ./modules/simpletest/tests/error.test ./modules/system/tests/error.test`;
`$cvs_rename ./modules/simpletest/tests/file.test ./modules/system/tests/file.test`;
`$cvs_rename ./modules/simpletest/tests/common.test ./modules/system/tests/common.test`;
`$cvs_rename ./modules/simpletest/tests/graph.test ./modules/system/tests/graph.test`;
`$cvs_rename ./modules/simpletest/tests/lock.test ./modules/system/tests/lock.test`;
`$cvs_rename ./modules/simpletest/tests/theme.test ./modules/system/tests/theme.test`;
`$cvs_rename ./modules/simpletest/tests/filetransfer.test ./modules/system/tests/filetransfer.test`;
`$cvs_rename ./modules/simpletest/tests/schema.test ./modules/system/tests/schema.test`;
`$cvs_rename ./modules/simpletest/tests/ajax.test ./modules/system/tests/ajax.test`;
`$cvs_rename ./modules/simpletest/tests/session.test ./modules/system/tests/session.test`;
`$cvs_rename ./modules/simpletest/tests/bootstrap.test ./modules/system/tests/bootstrap.test`;
`$cvs_rename ./modules/simpletest/tests/registry.test ./modules/system/tests/registry.test`;
`$cvs_rename ./modules/simpletest/tests/database_test.test ./modules/system/tests/database_test.test`;
`$cvs_rename ./modules/simpletest/tests/unicode.test ./modules/system/tests/unicode.test`;
`$cvs_rename ./modules/simpletest/tests/menu.test ./modules/system/tests/menu.test`;
`$cvs_rename ./modules/simpletest/tests/image.test ./modules/system/tests/image.test`;
`$cvs_rename ./modules/simpletest/tests/cache.test ./modules/system/tests/cache.test`;
`$cvs_rename ./modules/simpletest/tests/actions.test ./modules/system/tests/actions.test`;
`$cvs_rename ./modules/simpletest/tests/form.test ./modules/system/tests/form.test`;
`$cvs_rename ./modules/simpletest/tests/xmlrpc.test ./modules/system/tests/xmlrpc.test`;
`$cvs_rename ./modules/simpletest/tests/batch.test ./modules/system/tests/batch.test`;
`$cvs_rename ./modules/field/tests/field.test ./modules/field/field.test`;
`$cvs_rename ./modules/field/modules/list/tests/list.test ./modules/field/modules/list/list.test`;
#5
Subscribing. Would LOVE to see this done.
#6
I'm totally -1 on this. Tests are peripheral add-on stuff, not to be confused with the main code of something.
There are modules in contrib having multiple .test files. SimpleTest module itself has multiple .test files on behalf of other core functionality.
I want to keep on telling people to look at the primary files of a module/theme to understand it. Without "except, erm, that .test file, don't care about that".
#7
Hm?
To me, this is just harmonizing core with contrib. There is no way for a contrib module, regardless of how API-ish it is, to stick its tests in /modules/simpletest/tests.
Ergo, it doesn't make sense for System module to do it.
However, this is one of those sweeping changes that would've been much better to have been done a long time ago. Feels like D8 clean-up at this point.
#8
Totally with webchick on this.
A module's tests are part of the module - they should be in the same directory as the module.
#9
um, only now I realize two completely different proposals in this issue:
- Agreed with #4 (moving core subsystem tests from SimpleTest to System)
- Disagreed with #3 (moving .test files outside of ./tests subdirectories)
IMHO it's too early to move this D8 yet, but this task is too minor to. This entire sentence is too much.
#10
Good point.
Each module should be packaged with its own tests.
If a module (system) has multiple .test files, then they can go in a 'tests' subdir in the module's directory.
Would be nice if this could be D7 - is it just moving files?
#11
#4 is ready to go, not sure if it needs update, but that is the stuff dww needs to run on the backend...just needs approval. As for test files in/out of tests directory...we decided many many many many months ago that tests directory was for extra files (such as test modules or data files) and all tests should be placed in root of module directory in .test file. Being that the only argument against this is that somehow tests are to have lest priority while we are encouraging people to write tests in contrib I find that extremely contradictory.
Follow that idea we have /module/tests/module.test so we have a directory with a single file in it which is extremely stupid, thus the reasoning behind only creating the directory if extra files are needed. Based on Drupal 7's adoption of tests they are as much a part of a module as a .install file and should be treated as such. Not like they are easily mistake with the .test test extension...just as clear as .install...
#12
Contrary to that, when facing the need to add a hidden testing module to your module, or further .test files, you'd need to start moving files around.
Moving files is always a pain, unless you're working directly from drupal.org CVS/whatever, which is unlikely. None of our module files are currently designed to be moved at some point, due to some condition at any point in time. If such a condition can potentially exist, it's much more sane to build a long-term structure upfront, instead of afterwards.
You'll also simplify docs: "Testing code goes into ./tests, over and out." No excuses, no exceptions, no time restrictions or whatnot.
#13
The rule doesn't involve moving...core is the only place that should have multiple test files in a module. The rule we defined (no idea what issue it is in) was: all tests in .test file in root (if you need extra code include it from .test)...all other test related files in tests/ dir...never move .test file into tests dir. Core has the issue since we have the includes/ dir which doesn't have a place for tests other then system...only exception.
#14
Sorry, if I've been a bit snappy. Was a bit pissed off (about something else) earlier.
#15
I know of at least two contributed modules that started with a single .test file, but soon needed to add a hidden testing module, which obviously should not be contained anywhere else than in a ./tests subdirectory. Coder module, as another example, comes with multiple .test files. These instances alone (I'm sure there are more) are sufficient for me to disagree with a - in terms of these examples - illogical recommendation to put a .test file into the (module's/whatever's) root folder. Although we have all those testing files, actual testing coverage is most often still poor, so we naturally need to expect more code and files, not less.
But anyway, that discussion seems to be slightly off-topic here.
Re-reading the proposal in #4 for the 10th time, only now I recognize that it wants to move sub-system tests from simpletest/tests/ into "corresponding" module names. However, the name is the only thing that happens to match, so this proposal won't fly:
`$cvs_rename ./modules/simpletest/tests/path.test ./modules/path/path.test`;Those tests are for Path API in path.inc, which has little in common with Path [UI] module. It would be wrong to move any of simpletest/tests/*.test files into any module. Likewise, update.test contains tests for update.php, not Update module, completely different things.
Everything else, i.e., moving simpletest/tests/*.test files into system/tests/*.test, makes absolutely sense and +1, ideally for D7. Existing patches in the queue can be quickly corrected by editing .patch files directly.
I guess it would be wise to move discussion of our prior debate along with the following three lines into a separate issue:
`$cvs_rename ./modules/file/tests/file.test ./modules/file/file.test`;`$cvs_rename ./modules/field/tests/field.test ./modules/field/field.test`;
`$cvs_rename ./modules/field/modules/list/tests/list.test ./modules/field/modules/list/list.test`;
#16
This is an updated listed of the test files concerned by this issue using
find . -name "*.test" | grep "/tests/"(./modules/simpletest/tests/entity_query.test and ./modules/simpletest/tests/upgrade/upgrade.test have been added recently):# move modules .test files out of tests directory`$cvs_rename ./modules/field/modules/list/tests/list.test ./modules/field/modules/list/list.test`;
`$cvs_rename ./modules/field/tests/field.test ./modules/field/field.test`;
`$cvs_rename ./modules/file/tests/file.test ./modules/file/file.test`;
# move sub-systems .test file from simpletest to system module
`$cvs_rename ./modules/simpletest/tests/actions.test ./modules/system/tests/actions.test`;
`$cvs_rename ./modules/simpletest/tests/ajax.test ./modules/system/tests/ajax.test`;
`$cvs_rename ./modules/simpletest/tests/batch.test ./modules/system/tests/batch.test`;
`$cvs_rename ./modules/simpletest/tests/bootstrap.test ./modules/system/tests/bootstrap.test`;
`$cvs_rename ./modules/simpletest/tests/cache.test ./modules/system/tests/cache.test`;
`$cvs_rename ./modules/simpletest/tests/common.test ./modules/system/tests/common.test`;
`$cvs_rename ./modules/simpletest/tests/database_test.test ./modules/system/tests/database_test.test`;
`$cvs_rename ./modules/simpletest/tests/entity_query.test ./modules/system/tests/entity_query.test`;
`$cvs_rename ./modules/simpletest/tests/error.test ./modules/system/tests/error.test`;
`$cvs_rename ./modules/simpletest/tests/file.test ./modules/system/tests/file.test`;
`$cvs_rename ./modules/simpletest/tests/filetransfer.test ./modules/system/tests/filetransfer.test`;
`$cvs_rename ./modules/simpletest/tests/form.test ./modules/system/tests/form.test`;
`$cvs_rename ./modules/simpletest/tests/graph.test ./modules/system/tests/graph.test`;
`$cvs_rename ./modules/simpletest/tests/image.test ./modules/system/tests/image.test`;
`$cvs_rename ./modules/simpletest/tests/lock.test ./modules/system/tests/lock.test`;
`$cvs_rename ./modules/simpletest/tests/mail.test ./modules/system/tests/mail.test`;
`$cvs_rename ./modules/simpletest/tests/menu.test ./modules/system/tests/menu.test`;
`$cvs_rename ./modules/simpletest/tests/module.test ./modules/system/tests/module.test`;
`$cvs_rename ./modules/simpletest/tests/path.test ./modules/system/tests/path.test`;
`$cvs_rename ./modules/simpletest/tests/registry.test ./modules/system/tests/registry.test`;
`$cvs_rename ./modules/simpletest/tests/schema.test ./modules/system/tests/schema.test`;
`$cvs_rename ./modules/simpletest/tests/session.test ./modules/system/tests/session.test`;
`$cvs_rename ./modules/simpletest/tests/theme.test ./modules/system/tests/theme.test`;
`$cvs_rename ./modules/simpletest/tests/unicode.test ./modules/system/tests/unicode.test`;
`$cvs_rename ./modules/simpletest/tests/update.test ./modules/system/tests/update.test`;
`$cvs_rename ./modules/simpletest/tests/xmlrpc.test ./modules/system/tests/xmlrpc.test`;
# sub-system exception: upgrade.test was misplaced to begin with
`$cvs_rename ./modules/simpletest/tests/upgrade/upgrade.test ./modules/system/tests/upgrade.test`;
I agree with sun, any test file in simpletest/tests is there for a reason: it tests a sub-system and should not live in the module they share their name with (having modules and subsystems sharing the same name, e.g. path, can be confusing). so the renaming rule should be
`$cvs_rename ./modules/simpletest/tests/NAME.test ./modules/system/tests/NAME.test`;without any exception.#17
I'm going to be blunt here as (1) I'm not sure what the benefits of this cvs hackery are (history?), (2) I'm not sure who except dww can test this cvs script and (3) we're running the risk of never see this happen. So here is a simple script using git to generate a patch. (be patient if you're using dreditor).
#18
The last submitted patch, 779204_17_mv_tests.patch, failed testing.
#19
updated the git script to generate a patch compatible with
-p0and adding a script to clean up CVS (non tested).#20
Looks like there might some related fixes to do. I get the exact same error when I enable the testing module on my local environment:
PHP Fatal error: Class 'ImageToolkitTestCase' not found in .../modules/image/image.test on line 224.. This class is defined in ./modules/system/tests/image.test:73 (previously in ./modules/simpletest/tests/)#21
The last submitted patch, 779204_18_mv_tests.patch, failed testing.
#22
of course, the .info files files need to be updated! attaching this as a separate patch: 779204_22_align_info.patch. sending the combined patch 779204_22_mv+info.patch for testing.
#23
now that we have a working patch (and script), I can also confirm that the cvs script 779204_18_script_mv_tests_2_cvs.sh_.txt from #19 works on a local clone of drupal core cvs clone (see attached output).
#24
@score#16: I agree as well, it was not my intention to move the tests for includes to the module files.
As for the other discussion about root directory or not, there seems to be all this argument about moving files and accommodating for essentially the BIG cases, not the most common case that will also work just fine for the BIG cases. Secondly can we please not use coder as an example of what to do right...it is hardly my idea of the ideal contrib and breaks most of it's own rules and what not.
Looking at it's tests they are separated for no real reason. Each test case (30-50 lines on average) is put in its own file. I don't see us ever recommending something like that in core. Please keep in mind the common case for 90%+ of contrib will be module/tests/module.test with no other files. So they will have a directory with one file in it....nasty. For those who really really really need more files tests/yyy.inc and include from .test file (as recommended in proposal) works just fine. If a module really must (would have to have a TON of tests) it can put them in it's tests/ directory just like system does, but it isn't recommended.
I want to get this figured out since that was half my reason for filing this issue and as you see (like most issues) it takes forever to get through so I would prefer not filing another issues that won't make it until D8.
#25
This is required to allow the SimpleTest contrib module to override the core module, without having to entirely duplicate all tests or use whack workarounds.
#26
+++ modules/field/field.info
@@ -12,6 +12,6 @@ files[] = field.default.inc
-files[] = tests/field.test
+files[] = field.test
+++ modules/field/modules/list/list.info
@@ -5,5 +5,5 @@ package = Core
-files[]=tests/list.test
+files[]=list.test
+++ modules/file/file.info
@@ -7,4 +7,4 @@ core = 7.x
-files[] = tests/file.test
+files[] = file.test
Can we please skip these changes for this issue? I still think it's wrong to do that, as we're constantly hitting the same problem throughout Drupal core issues: more and more modules need their own testing *assets*, which may be further .test files, entire testing modules, arbitrary testing files, or whatever. It makes much more sense to keep everything together in a ./tests subdirectory.
But anyway, can we simply skip those changes here?
Powered by Dreditor.
#27
This should be done before beta, I think. Though not sure whether moving those files is able to break anything.
#28
Reasons this still needs work:
A) I agree with sun #26 about not moving module.test out of */tests if that's already there, at least not as part of this issue.
B) If we're moving the .test files themselves for the core API out of simpletest/tests into system/tests, why not all the support files (module, info, upgrade DB) that those tests depend on, too? That seems like at least as much of a WTF if the test classes in system/tests/*.test still all depend on files from simpletest/tests.
I'm not sure on the best timing for this, either. But, I'd be happy to make it happen whenever webchick et al want.
I'd like to add a new test file for system (authorize.test, see #609772: Impossible to extend the FileTransfer class system in contrib). Should I just go ahead and add that at system/tests/authorize.test in the separate patch, so we at least get the directory started? Should I just stuff the tests in system.test for now and move them into a separate file after this? Just leave them in system.test and system_test.module forever?
#29
I guess we're too late to move these file around now in D7?
#30
No, this is actually required.
Otherwise, developers cannot upgrade to a SimpleTest 2.x from contrib. All files not belonging to the SimpleTest module itself need to go.
#31
Can't simpletest 7.x-2.x do a hook_theme_registry_alter() to get these included?
#32
The issue activity heavily disagrees with the 'major' marker and catch has a workaround there.
#33
#34
It'd be good to revive this. Completely agree the issue should be restricted to moving simpletest/tests/* to system/tests/*.
If everyone still agrees, could we try to schedule that for soon after #22336: Move all core Drupal files under a /core folder to improve usability and upgrades. Maybe November 4th or something?
Eventually it'd be great for libraries in /includes to be able to include their own /tests folder (at least for unit tests), but that probably requires the PSR-0 conversion to happen first.
#35
sure, November 4th then.