Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
This needs discussion because we have a lot (maybe 200) classes that need to be placed somewhere...
We should discuss the approach here!
Remaining tasks
- #1597630: Convert actions.test to PSR-0
- #1598548: Convert ajax.test to PSR-0
- #1598552: Move batch tests to the right directory
- #1598556: Convert bootstrap.test to PSR-0
- #1598558: Convert cache.test to PSR-0
- #1598564: Convert common.test to PSR-0
- #1598568: Convert database.test to PSR-0
- #1598570: Convert error.test to PSR-0
- #1598574: Convert file.test to PSR-0
- #1598576: Convert filetransfer.test to PSR-0
- #1598578: Convert form.test to PSR-0
- #1598582: Convert image.test to PSR-0
- #1598584: Convert installer.test to PSR-0
- #1598586: Convert lock.test to PSR-0
- #1598588: Convert mail.test to PSR-0
- #1598590: Convert menu.test to PSR-0
- #1598592: Convert module.test to PSR-0
- #1598594: Convert pager.test to PSR-0
- #1598596: Convert password.test to PSR-0
- #1598600: Convert path.test to PSR-0
- #1598602: Fix the path of Queue tests
- #1598606: Convert schema.test to PSR-0
- #1598608: Convert session.test to PSR-0
- #1598610: Convert symfony.test to PSR-0 and remove all the files[] instances in system.info
- #1598614: Convert theme.test to PSR-0
- #1598616: Convert unicode.test to PSR-0
- #1598618: Convert update.test to PSR-0
- #1598620: Convert uuid.test to PSR-0
- #1598622: Convert xmlrpc.test to PSR-0
- #1619898: Convert upgrade tests to PSR-0
Comment | File | Size | Author |
---|---|---|---|
#49 | drupal-1593058-49.patch | 880 bytes | tim.plunkett |
#41 | 1593058-system-tests-41.patch | 6.25 KB | aspilicious |
#40 | 1593058-system-tests-40.patch | 6.18 KB | aspilicious |
#37 | 1593058-system-tests-37.patch | 11.08 KB | aspilicious |
#32 | 1593058-system-tests-psr0-32.patch | 258.18 KB | aspilicious |
Comments
Comment #1
BerdirAs suggested in IRC, It would be great to be able to move most if not all of them to the Drupal\Core namespace.
The easiest would be to introduce Drupal\Core\Tests (or even Drupal\Tests) and then e.g. have Drupal\Core\Tests\Database\SelectQueryTest. That would just require a single, static classloader extension and path to look for test files..
Comment #2
RobLoachHere's an example of the transition:
CommonAutocompleteTagsTestCase -> modules/system/lib/Drupal/system/Tests/Common/AutocompleteTagsTest.php
CommonDrupalHTTPRequestTestCase -> modules/system/lib/Drupal/system/Tests/Common/HttpRequestTest.php
FormElementTestCase -> modules/system/lib/Drupal/system/Tests/Form/Element.php
FormsElementsTableSelectFunctionalTest -> modules/system/lib/Drupal/system/Tests/Form/ElementsTableSelectFunctionalTest.php
SessionHttpsTestCase -> modules/system/lib/Drupal/system/Tests/Session/HttpsTest.php
[EDIT] Actually, we could move those to Drupal\Core\Tests\* when appropriate. session.test's SessionHttpsTestCase becomes core/lib/Drupal/Core/Tests/Session/HttpsTest.php.
Comment #3
aspilicious CreditAttribution: aspilicious commentedit's funny that we alrdy have a "tests" folder, but not "Tests"
Comment #4
RobLoachcore/tests
core/lib/Drupal/Core/Tests
Taking those to things into consideration, what if we were to just stick them in the system module's lib directory for now, and consider moving them later on. Spliting the files into PSR-0 classes seems like the first step. We can worry about the "proper" location later on as it's easier to move files. I've put together #1597630: Convert actions.test to PSR-0 as a start.
Comment #5
Crell CreditAttribution: Crell commentedTest for OOPified code in /lib should, I suppose, live with it, or at least somewhere in /lib.
Tests for procedural code in /includes, shouldn't that go in simpletest, not in system? Either way, I'm fine with that being tied to a module for now.
Comment #6
RobLoachI like the idea of it going in modules/simpletest/lib. Set #1597630: Convert actions.test to PSR-0 to needs work.
Comment #7
BerdirWe just moved them from simpletest to system, do you really want to revert that? See http://drupal.org/node/1540510
Comment #8
RobLoachLet's keep them in system module then! Added issues for each task in the issue summary.
Comment #8.0
RobLoachtasks
Comment #8.1
RobLoachUpdated issue summary.
Comment #9
sunComment #10
sun1) Please make sure to (re-)roll all of those patches with:
2) Almost every of those patches will break major efforts happening in parallel currently. I wonder whether it wouldn't make sense to defer all of them to code freeze - or at least, a single, coordinated date.
Comment #12
RobLoachRebasing to update/reroll patches is quite simple. rfay's article and xjm's notes on it are great. If conflicts do occur, it is easy to reroll these PSR-0 patches. If you have any particular conflicts in mind, please feel free to post notes about them in their respective issues so that we can coordinate resolutions.
The PSR-0 RTBC queue is up and growing. It's up to catch to commit them, and he has been doing so.
Comment #13
webchickJust a note that there is the Avoid commit conflicts tag for specifically flagging issues that are a) *really* close to (or at) RTBC, and b) a complete and utter pain in the patootie to re-roll. Core committers will check this queue for conflicts before going on a commit spree.
Comment #14
aspilicious CreditAttribution: aspilicious commentedNo I *completly* disagree with this. Every issue in this list conflicts with it self so we have to commit this one by one. So it will take weeks before every item on this list is done.
Btw if this gets in you have to reroll those conflicted patches once. Every time a test gets committed we have to reroll these patches. So in the end you just say, let aspilicious reroll his patches 30 times because this is more important.
I don't realy get the fuzz about waiting untill code freez, everyone knows we have lots of work to do than to clean everything so why not doing as much as possible when we can. Rerolling an importing patch maybe takes 20 minutes but in Drupal land 20 minutes is *nothing*.
AND last thing before I shut up, once this is done every test class sits in its own file so there will be less conflicts in the end. NOW chances are big test files conflict because of this 4000+ line test file madness.
Comment #15
BerdirWhat's more important is that we need to convert all classes to PSR-0 so that we can remove the registry, we can't do that in the code cleanup phase.
Comment #16
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedNote that Crell has asked catch to wait with commiting PSR-0 patches at least until #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel is in (May 31). He probably had forgotten that when he committed one.
Comment #17
aspilicious CreditAttribution: aspilicious commentedAdded the tag webchick proposed and made a list of affected test files to that issue.
So we still can commit other test conversions. Not every file is block on that one.
Comment #18
Crell CreditAttribution: Crell commentedI agree that we don't want to delay this migration too long, because it does (should?) make things easier after it's done.
What about scheduling them in pieces? Vis, "node module's test shifting goes in X date, so everyone beware, but you can still mess with user module this week". "OK, user module's tests get moved around this week, FYI, but you can still hack away at block module." Etc.
That at least means we don't need to chase head on 30 test-move patches at a time, just one at a time.
Comment #19
aspilicious CreditAttribution: aspilicious commentedThere are to many patches that depend on various subsystems, you can't postpone a single test file. Catch has pushed some tests alrdy so it's going well.
Comment #20
aspilicious CreditAttribution: aspilicious commentedThere are to many patches that depend on various subsystems, you can't postpone a single test file. Catch has pushed some tests alrdy so it's going well.
Comment #21
catchfwiw I've committed a bunch of PSR-0 patches after aspilicious posted a list of ones that conflict with the kernel patch, there's currently no other active patches tagged with 'avoid commit conflicts', and the conflicts created by these patches do not make for difficult re-rolls for other patches - it's not like committing a big API or coding standards change all at once.
Apart from that patch, I don't see any reason to delay doing this migration in general. Once we're able to remove the registry,
#534594: [meta] system info cache writing and registry rebuilding is broken without a full bootstrap becomes mostly a D7 issue, so that's one less (very, very hard to fix) critical bug against 8.x. While it's a very long way around to get rid of that bug, this is the way we've decided to do it, so PSR-0 conversion is a release blocker unless that issue gets magically out of limbo soon.
While tests for procedural code should stay in system (we moved them there because simpletest module should really just be a UI for running simpletests, ideally it wouldn't have DrupalUnitTestCase or DrupalWebTestCase in it either), it'd be great if we could add support for including test code with stuff in /Core and Component, and then move classes there - would save doing the move-around twice.
Does that need its own issue?
Comment #22
sunThe latter should be fairly easy to do, since we've introduced a single helper function for Simpletest to discover the PSR-0 tests, and yes, dedicated issue, please.
Comment #23
catchOK, opened #1605512: Allow stuff in core/lib/Drupal to ship with tests.
Comment #24
catchAnd marking this postponed on that. If we can move all those tests again once, it'd be good.
Comment #25
catchDoh, ignore that. This only counts for stuff in lib, not procedural code, so it's not actually that many tests anyway.
Comment #25.0
catchUpdated issue summary.
Comment #25.1
aspilicious CreditAttribution: aspilicious commentedAdded upgrade tests
Comment #26
aspilicious CreditAttribution: aspilicious commentedOk we decided to leave out the .info changes for now because they are conflicting all the time!
Ow yeah!
We have to clean those in the end!
Comment #27
aspilicious CreditAttribution: aspilicious commentedTheme.tests reveiled that we have also have to convert the ThemeRegistry class found in theme.inc.
We shouldn't forget that ;)
Comment #28
RobLoachUpdating the title to reflect that :-) .
Comment #28.0
RobLoachRemoved cruft issues
Comment #29
aspilicious CreditAttribution: aspilicious commentedHere is a patch for system.test itself. It's a complicated patch due to renames and different namespaces. You should apply it to see how it's structured.
If I did something completly wrong tell me :)
Comment #31
aspilicious CreditAttribution: aspilicious commentedThis should be green...
Comment #31.0
aspilicious CreditAttribution: aspilicious commentedRemoved crufty issue
Comment #32
aspilicious CreditAttribution: aspilicious commentedRerolled!
Comment #33
RobLoachThese are already PSR-0, I do enjoy a good namespace cleansing though!
After looking through the patch and checking through the file definitions, I'm liking this patch more and more. This patch is huge, in terms of both its size and its awesomeness. Well done! RTBCed.
Comment #34
RobLoach#32: 1593058-system-tests-psr0-32.patch queued for re-testing.
Comment #35
catchCommitted/pushed to 8.x, nice work!
Leaving this open since we still have the files[] entry at the end.
Comment #36
RobLoachSetting to postponed until the other patches get in :-) .
Comment #36.0
RobLoachUpdate info
Comment #37
aspilicious CreditAttribution: aspilicious commentedDid a grep to check if we had every instance of "files[]" and apparantly there are some sneaky instances left. This patch should fix those... Hopefully...
Comment #38
BerdirThe stream wrapper changes in file_test.module are already part of #1598574: Convert file.test to PSR-0.
Comment #39
BerdirMaybe "MockFileTransfer" or something like that? Would also match the comment then. Otherwise this is once again an extremely generic class name, which is already used: http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21FileTransf...
Should use fully qualified class name, also specifiy the type for @return, the one in system is missing documentation.
Comment #40
aspilicious CreditAttribution: aspilicious commentedComment #41
aspilicious CreditAttribution: aspilicious commentedThis one is better
Comment #42
RobLoachMockFileTransfer class.
MockFileTransfer class, round two :-) . Do we really have two different classes named the same in two different modules? I understand it doesn't actually cause a problem, but we could rename one of the Update module one to UpdateMockFileTransfer, or something.
Comment #43
aspilicious CreditAttribution: aspilicious commentedThey do exactly the same thing (just a different title). Isn't it normal they have the same name than? I don't have any preferences...
Comment #44
RobLoachYeah, they're in different namespaces, so there arn't any class name conflict anyway. It's good to clean up the dependency here as well. Having update_test depend on system_test module is not the correct way around this, what is in the patch here is the correct way around it.
Comment #45
catchReally? Can we think of somewhere else to put these?
Comment #46
RobLoachThe class was in the system_test.module itself before. Seems like the logical place to put it is in system_test's namespace. Have any other ideas on where it could go?
Comment #46.0
RobLoachUpdating the list
Comment #47
webchickAll the other RTBC PSR-0 patches came in, so applied and committed this patch. YAY.
Committed and pushed to 8.x. WOOHOO.
Comment #48
catchWe've still got to actually remove the files[] entry from system.info when the last tests have landed. Re-opening for that.
Comment #49
tim.plunkettCleaning up the .info file, but it shows that we still have two tests to convert?
Comment #50
aspilicious CreditAttribution: aspilicious commentedSrry, see #1598610: Convert symfony.test to PSR-0 and remove all the files[] instances in system.info
Comment #51
BerdirYes, registry.test is removed in the registry removal patch and symfony.test makes no sense anymore, because when PSR-0 wouldn't work then we couldn't execute any tests in the first place, as discussed in the issue link in #50.
Comment #52.0
(not verified) CreditAttribution: commentedUpdate