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.
We can start with this once #1477218: Convert Tracker tests to PSR-0 / PSR-0 test class discovery is commited I guess.
Part of #1513210: Meta: Start converting module provided classes to PSR-0
Comment | File | Size | Author |
---|---|---|---|
#55 | drupal-1541676-55.patch | 397 bytes | tim.plunkett |
#48 | simpletest-psr0-1541676-45-no-upgrade-tests.patch | 179.86 KB | tstoeckler |
#45 | simpletest-psr0-1541676-45.patch | 182.2 KB | Berdir |
#44 | 1541676-44.txt | 11.27 KB | tstoeckler |
#43 | simpletest-psr0-1541676-43.patch | 205.71 KB | Berdir |
Comments
Comment #1
BerdirThis is a bit tricky, more later.
Anyway, I have some code lying around (Actually, more than that, I can successfully run tests) so assigning to me to get the lock :)
Comment #2
BerdirIn case someone is up for some bike shedding:
Any objections?
Comment #3
BerdirYay, TrackerTest is commited.
So, the main issue here is that we need to add a use statement to all .test files and change the extends bit of all test class definitions.. I think it's best to do this before starting to convert the test code or we will have to first add a use for \DrupalWebTestCase and then fix it all over the place. Doing it this way round allows it to do it correctly from the beginning when moving the test code to PSR-0.
I was able to execute some example tests both in the UI and with run-tests.php, let's see what the testbot thinks. It's possible that a seldomly used method still has a missing use or something like that. Also possible that the testbot doesn't like this at all.
Comment #4
BerdirUps, the above patch contained a test file, removed that.
Comment #6
BerdirStrange, no idea why that function call is trying to be executed within the namespace. Let's try this for now.
Also, not sure about the locales_source not found pdo exception...
Comment #8
BerdirBecause the function was defined in drupal_web_test_case.php and I forgot to move it.
Changed namespace from Drupal\simpletest\Tests to Drupal\simpletest because, as discussed with sun, these aren't tests, they are the API for other modules.
Let's see if this is any better.
Comment #9
neclimdulAt the cost of possibly opening a bikeshed, I've got 2 technical questions about PSR-0'd tests.
Drupal\system\Tests\SubsystemUnitTest
registered by the system module?Drupal\system\Tests\SubsystemUnitTest
, how would that unit test use mock objects? Would my mock objects live inDrupal\system
orDrupal\system\Tests
as well or would I be responsible for registering a namespace manually or containing all mock objects in the test file?Note: Since SimpleTest runs in a long running parent process the registering of namespaces is permanent unless the test has its own loader. Also auto-loading can't be undone so testing of loading functionality is in someways impossible or at best fragile. This isn't new though just to be considered.
Comment #10
kika CreditAttribution: kika commentedTime for a blast from the past: #249553: Rename SimpleTest to Testing (test.module). Should it be re-considered now?
Comment #11
xjmI am not sure this issue belongs on http://drupal.org/node/1543796, because the base classes aren't test classes; they're API. (As we've pointed out in other contexts, but I think this issue should eventually have its own change notification instead.) I've left it on there for now as a reference or whatever, or in case I'm wrong. :)
Comment #12
BerdirI did add there because once this has been commited, the examples there should be updated to use the new class names and namespaces IMHO. So it did make sense to me to add this one as a related issue. That it deserves it's own separate change record is possible, maybe it can also be grouped with something else.
Comment #13
Berdir#8: convert-simpletest-to-psr0-1541676-8.patch queued for re-testing.
Comment #15
BerdirOk, here is a re-roll.
@neclimdul: There have been discussions on extending the test class search to core/lib so that tests can be moved out of system.module. But yes, right now, you'd do that. Not sure about the mock objects, in the test namespace would probably make sense. However, this is not relevant for this issue, the detection and handling of PSR-0 tests has been defined in the TrackerTest issue, your questions should probably be handled in a new issue or maybe the meta issue linked above. All this issue does is converting the base test classes to PSR-0. Updated the title to clarify that.
@kika: Same, that shouldn't happen here imho but stay in that issue. If we decide to do that, then all we need to do is a search & replace for Drupal\simpletest to Drupal\testing (additional to converting the module itself) once this is in.
Patch is this time with git diff -M. Half the size, but imho less readable.
Comment #17
sunThanks! This looks almost ready to me.
Typo ;)
I don't think these are necessary.
I never understood why these two are procedural functions. Should be tacked onto WebTest. But, separate issue.
Is this necessary? If so, the comment should state why.
Doesn't look like it is needed though, because the equivalent change is missing in run-tests.sh, but testbot still executed.
Comment #18
BerdirThanks for the review.
- Fixed the weird typo.
- The uses are necessary, otherwise PHP will look for e.g. Drupal\simpletest\PDO. The alternative would be using \PDO but according to the coding standards, this is only allowed if something is only used once, which is not true for most of them and core already hase various use PDO;'s.
- Weird, I am pretty sure the \\ hack was necessary and I only added it after it failed. But yes, you are correct, it is in the .module file and it works just fine without. Removed.
Comment #20
BerdirRe-rolled.
Comment #22
BerdirHm, looks like I forgot to save one of changed .test files previously.
Comment #23
RobLoachLooks like we're moving simpletest_verbose() loosely into WebTest.php? Could we move that function in either as a function of Test or WebTest itself? Having loose functions in PSR-0 PHP files will mean the function won't be available unless we autoload the WebTest class. Seems like something we want to avoid. Let's just move it into the Test::verbose() function itself.
-28 days to next Drupal core point release.
Comment #24
RobLoachLooks like it was in both simpletest.module and WebTest.php. simpletest.module it is. Also removed an empty whitespace.
Comment #25
sunThis patch looks ready to me. But if possible, I'd like to do #249553: Rename SimpleTest to Testing (test.module) first (resulting in a simple in-patch-file rename here).
Comment #26
sunScratch #25. Don't want to block this.
Comment #27
sun#24: 1541676.patch queued for re-testing.
Comment #30
BerdirRe-rolled. An interdiff doesn't make much sense I think, I mostly fixed merge conflicts (quite a few of them) and adapted for the changes to the setUp() method. Also renamed all references to WebTest in docs to use the fully qualified name. I found them searching for " WebTest::".
Comment #31
aspilicious CreditAttribution: aspilicious commentedComment #32
tstoecklerThis is an error from the merge. SignedFileStorage no longer exists.
Yay!
This shouldn't be needed.
This should wrap after "string" due to the longer method name above.
The "in this file" part is no longer accurate.
Seems like these are all not needed.
Comment #33
sunIdeally, we should account for #1567920: Naming standard for abstract/base classes
Comment #34
BerdirThanks for the review, looks like you actually went through it, awesome ;)
- Remove the merge mess in config.test. The profile is now testing by default, re-removed that as well ;)
- Updated the docblocks and removed the unecessary uses. I think I just added that one to all .test files without looking if it's actually needed ;)
- Added Base suffix to Test/UnitTest/WebTest.
Comment #36
BerdirAnd another new test class :)
Comment #38
tstoecklerThose don't match. Do we really want PollWebTestBaseCase though?
It seems that #1567920: Naming standard for abstract/base classes would imply PollWebTestBase, IINM.
Comment #39
BerdirYeah, that was a wrong search & replace rename, sorry about that. Should test that stuff locally :)
Renaming the actual test classes will be part of the PSR-0 conversion of them.
Comment #41
BerdirMissed some Test -> TestBase renames...
Comment #43
BerdirOk, one last remaining WebTest in there, this one should be green! I hope...
Comment #44
tstoecklerAlright this review was a bit lengthy, so I dumped it into a text file in order to not spam this issue too much. Most of the issues were unnecessary renames of Base (or not base...) classes.
Comment #45
BerdirOh wow, that rename went totally sideways :( Sorry about that!
- Yeah, I noticed the weird stuff in CommentNodeAccessTest as well. I'm not sure but it might even be trying to go around CommentHelperCase::setUp() explicitly, because it does a node_access_rebuild() between the setUp() and creating the users/node. I'd agree separate issue though, because I would like us to start requiring an array for setUp() and stop dancing around :) sun even has an issue/patch where he moves the module declaration to a class property.
- Wasn't sure about the abstract WebTestBase but it makes sense to me.. it doesn't make any sense to instantiate that class directly. Also noticed that TestBase calls setUp() and tearDown() so these should be declared as abstract methods there, but I think that should be done in a follow-up.
- Removed all wrong Base's. Search for "Base extends" only gives me WebTestBase and UnitTestBase, searching for "BaseCase" returns no results. So I hope I got all of them.
Now we're almost back at the original file size. That should have made it obvious that something went wrong :)
Comment #47
Berdir#45: simpletest-psr0-1541676-45.patch queued for re-testing.
Comment #48
tstoecklerI've read that one time too many now. Opened #1569856: Make CommentHelperCase support $modules like DrupalWebTestCase and make CommentNodeAccessTest use that
These hunks basically revert #1352000: Forward-port upgrade test clean-ups from 7.x to 8.x. I personally don't know why they were removed, but I guess it was for a reason.
Either way this patch is RTBC. I've attached a patch, which is exactly (!) the same as the above, I just deleted those two hunks from the patch file. It just needs to be decided, which one is correct.
Please do not revert RTBC status, unless there is something seriously wrong with this patch. I've read through the entire patch 3 times now, and it's really a pain. So if someone finds coding standards violations or something (I didn't), this is definitely a case where that would warrant a follow-up.
Comment #49
sunI've reviewed all changes of #48 in detail. Truly RTBC.
Comment #50
catchCommitted/pushed this one so it doesn't get broken again. This will need to be added to the overall PSR-0 change notice.
Comment #51
BerdirI have updated http://drupal.org/node/1543796, it makes IMHO sense to make that a general "How to change test classes from 7.x to PSR-0/8.x". Added a block about the renamed base test classes, added a point about adding a Base suffix to module base test classes and updated the examples.
Not sure about the general PSR-0 part of this, should this be added to http://drupal.org/node/1320394? Not sure if it makes sense to list all modules that have classes there, it's kinda obvious and unless we'd add all classes within that module, not really helpful because they have been renamed as well.
Comment #52
aspilicious CreditAttribution: aspilicious commentedWatch out with backslahses in change notices! They dissapear sometimes.
Comment #53
sunThanks! Change notice looks good to me. (minimally tweaked it)
Comment #54
webchickComment #55
tim.plunkettBriefly reopening this, the files[] entry for drupal_web_test_case.php was left in.
Comment #56
BerdirUpsie. Yep, that needs to go.
Comment #57
catchYep. Committed/pushed.