Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Currently there are 7 test entity types all using the same class.
This was hacky and they should have been subclasses to begin with.
Because of #1763974: Convert entity type info into plugins, it's now required, and that issue makes the necessary changes to their structure, but exposes and adds some documentation inconsistencies.
Comments
Comment #1
BerdirI think they should actually be moved over the entity_test.module, that's what I've been trying to do for a while but there's always been stuff that's blocking it. Right now, we still have inconsistencies in regards to revision support of them.
Comment #2
fagoI agree that this should probably all be converted to the entity_test entity type. I don't see a need for having duplicating test entities - if we need multiple variations of test entities we can do so in entity_test module (and I think das_peter has been working on that already).
Comment #3
BerdirExactly, #1831444: EntityNG: Support for revisions for entity save and delete operations adds generic revision support and enables revision-support for entity_test and #1833334: EntityNG: integrate a dynamic property data table handling provides generic data table support and comes with a handful test entities to test every possible combination of these capabilities.
Those combined should be able to replace most of test_entity and the relevant test code that relies on it. Two exceptions that I'm aware of:
- Bundle CRUD functions and hooks. There are issues about improving this but I'm not sure what the exact state of them is.
- Support for editing multiple entities in the same form by the field system.
Comment #4
BerdirI started to work on this, but there is an insane amount of tests to to through. New tests like edit.module are using test_entity instead of entity_test which means that we need to clean those up :(
And a lot of tests need various field types to be converted to NG, I started to write patches to various ones, see #1732730: [meta] Implement the new entity field API for all field types. Please review :)
Comment #5
BerdirOk, here is a start. A bunch of search and replace of test_entity/test_bundle to entity_test + fixed the field types tests. Tons of duplicated tests in there for various field types which all are doing the same strange things :)
Will still have a lot of fails remaining, but I'd like to see which ones.
Notes:
- the field_test entity types do a bunch of things that we do not yet support in entity_test, like creating bundles. In fact, the bundle API (rename, delete, ..) still lives in field.module.
- It's currently a bit confusing because some field types have been converted to NG yet and others have not. I'm working on converting the field types in parallel and need reviews there, especially on the file item one :)
- Made a bunch of conceptual changes, notably the changes to option list callback allowed values. That relied on fixed id's and revision id's because there was nothing else but that required very complicated support for creating new revisions with fixed id's. I changed to use label/uuid/uri/bundle, which are fixed and guaranteed to be different. I think that is much easier to understand now.
Comment #6
BerdirAlso, field_test has a bunch of specific entity types like these: "BundleKeyTestEntity BundleTestEntity CacheableTestEntity LabelCallbackTestEntity LabelTestEntity NoLabelTestEntity". I'm not sure if we should keep them and just move them over to entity_test or merge their specific things into existing entity types. E.g. add a label callback to one of the existing entity_test types? Might be cleaner to just move them over.
The label ones for example are related to the EntityPropertiesTest, that I'm moving over to the other entity tests in #1893108: Convert most entity tests to DrupalUnitTestBase, I guess I can move those as part of that.
Comment #8
BerdirUpdated patch, a number of tests fixed but still a lot to do. Will probably be easier after the field types have been converted.
Comment #10
BerdirContinued to work on this a bit.
This is a lot of work. Bundles, weird stuff with revisions and multiple values that I haven't yet figured out and more.
However, most of it is stuff that we need to do anyway. Changing from test_entity to entity_test is really the easiest part.
Comment #12
BerdirWork in progress, worked on the field form tests.a bit and re-rolled.
Seeing some issues that I hope will get resolved with the improvements to the BC decorator from the node NG issue.
Comment #14
BerdirAnother work in progress patch.
Worked a bit on DatetimeFieldTest and noticed that the DateTimeItem is completely broken as value is a date and is never saved.
That file is moved because it has a wrong name (Date*T*imeFieldTest.php). Will probably have to open a separate issue for that.
Comment #16
BerdirCombined with the NodeNG patch to see how this affects the test failures.
Comment #18
BerdirUpdated with the latest version of the NodeNG patch. Fixed a number of bugs in the test, added view modes for entity_test and a new revision checkbox in EntityTestFormController (this might even be something that we could provide by default?)
Comment #20
BerdirStarted to add bundle support to entity_test, most of the remaining test failures are related to that.
Might have a few additional failures in other places due to the changes.
Comment #22
BerdirInteresting, looks like entity translation permission names fall apart for entity_test_mul if the bundle doesn't match the entity_type.
Comment #24
BerdirRe-roll after Node NG and Field API DUBT conversion went in. Might have some additional test fails.
Comment #26
BerdirAdded bundle definition/column to the other entity_test types and updated the entity tests accordingly.
Comment #28
BerdirAnother re-roll and a few test fixes.
Removed the rename of the datetime test filename as it makes it hard to review the changes and it only breaks the UI, not the actual tests.
Comment #30
BerdirAnd the patch keeps getting bigger :(
Let's see how many fails are left, should have fixed quite a few.
Comment #32
podarokgood to see here interdiffs for easy review during process
Comment #33
BerdirThis is just me messing around, too early to do reviews and too early to provide interdiffs :)
Comment #34
podarokanyway interdiff can be a good helper for injecting new developers and discussions
Comment #35
BerdirTagging.
Comment #36
BerdirRe-roll.
Comment #38
amateescu CreditAttribution: amateescu commentedWith @Berdir's approval, I extracted the part about bundle support for EntityTest into a separate, smaller patch in #1971668: Add bundle support to EntityTest* entity types because I need it for rewriting some Entity Reference tests.
Also, the patch from #1974474: Make sure within field_attach_*() that we are working with a BC entity would make our lives a lot easier here.
Comment #39
andypostIs it possible to split this tremendous patch into parts? this one stops #1953410: [Meta] Remove field_create_*(), field_update_*() and field_delete_*() in favor of just using the ConfigEntity API and others
Comment #40
webchickThat's just blocking a normal refactoring, so this is also normal.
Comment #41
BerdirThe split-up is already happening. See #38 for two issues, one of those already got in. Other split-ups are quite possibly, e.g. the field types, for which I'm thinking about a generic base class similar to e.g. the entity translation UI tests. Most of those were obviously copied and adjusted slightly, lot's of duplicated changes necessary in there.
This is more a personal playground to see how far we're off from removing test_entity.
Comment #42
BerdirRe-roll and removed the now unecessary getBCEntity() calls.
As said before, will try to extract parts of this, e.g. the field type tests into smaller issues.
Comment #44
BerdirNice try git but that wasn't what I wanted to be merged there.
Comment #46
BerdirThat search & replace was wrong. Ended up with "$entity()". This should be better. Note that this will possibly have more fails but should have less failing classes.
Comment #48
BerdirSome more test fixes.
Comment #50
xjm#1953408: Remove ArrayAccess BC layer from field config entities is apparently postponed on this, and that's critical, so... so is this now! Congratulations! :D
Comment #51
BerdirYay ;) This is already part of the critical convert entities to NG meta, so it doesn't really matter :)
Here's a re-roll. As said above, still plan to split this up into multiple issues.
Comment #53
Berdir#51: convert-test-entity-to-entity-test-1822000-51.patch queued for re-testing.
Comment #55
BerdirRenamed a bunch of new references to test_entity/test_bundle which should fix quite a few of those test failures. Also removed the entity types completely for now that we no longer want, that might introduce a few new failures.
Comment #57
BerdirFixed some more tests, merged #1957888: Exception when a field is empty on programmatic creation which also fixes various things. Let's see what's left...
Comment #59
amateescu CreditAttribution: amateescu commentedI worked a bit on some of the remainig failures, but I'm not done yet so uploading only an interdiff for now.
Comment #60
BerdirThis is not the same necessarly.. the clone keeps the same id's, createDuplicate() removes them.
I think it does clone to be able to call field_attach_load() and similar multiple times on the same entity.
Comment #61
BerdirThis should pass the options widget test. Next step: Extract all non field API test group related changes into a separate issue.
Also included most of the interdiff from above except the clone/createDuplicate8) part.
Comment #63
BerdirRevert the ItemList change and instead filter in the test. That should fix some of the fails in other issues.
Comment #65
BerdirAs promised, extracted all changes that aren't relevant to "Field API" test group into #2004224: Convert tests using TestEntity to EntityTest (except Field API tests), to make it easier to review those parts.
Comment #66
andypostcommited #2004224: Convert tests using TestEntity to EntityTest (except Field API tests)
patch should be much easy for review
Comment #67
BerdirInitial re-roll, will probably explode :)
Also updating title.
Comment #69
BerdirThis should get us back to where we were before.
Remaining cases:
- Multi-Entity forms. I think @yched looked into that already in the field types issue?
- Handling of NULL/empty arrays with BC decorator, that breaks both the field attach storage and the field sql storage tests.
Comment #71
yched CreditAttribution: yched commentedYup, I'll merge in the fixes for the nested form from the field type patch tomorrow.
Comment #72
BerdirI tried that as well (FormTest.php, NestedFormTest.php, field_test.admin.inc changes) but wasn't able to get the nested form tests to pass, got some weird ajax add more problems where the field was missing. Is there more that I need to merge?
Comment #73
effulgentsia CreditAttribution: effulgentsia commentedNot sure if it's related, but while working on #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets, I found that on my local MAMP (PHP 5.4) install, NestedFormTest would fail in mysterious ways due to faulty unserializing of object references within $form_state. However, bot seemed to not have that problem, so my local failures did not happen on bot.
Comment #74
amateescu CreditAttribution: amateescu commentedFor the record, I have the same failures locally as #73 on XAMPP, PHP 5.4.4.
Comment #75
yched CreditAttribution: yched commentedFixes the nested form test.
Comment #77
BerdirTagging.
Comment #78
BerdirFixing the remaining form tests, working on translation test but that's above my head. Also resolved merge conflicts in config import tests.
Comment #80
effulgentsia CreditAttribution: effulgentsia commented#78: convert-test-entity-to-entity-test-1822000-78.patch queued for re-testing.
Comment #82
BerdirRe-roll.
Comment #84
plachI started looking at this but couldn't finish yet, sorry.
Comment #85
plachThe problem with the field translation tests seems to be double: to assign multiple field languages to an entity we need it not be language neutral. This is the easy one. The other one is that we are trying to pass an NG entity to
_field_invoke()
which does not know how to deal with it. It seems we need to use the BC decorator :(The attached interdiff fixes some failures, didn't look to the others but I suspect they are related.
Edit: as you can see I worked on
testFieldInvoke()
;)Comment #86
BerdirThanks!
Made a few more changes and also updated invoke multiple. Killed the hashing/serializing and just add the entity uuid to the return value. That makes it *much* easier to debug that test.
Also fixed most failures in save/load, just something with default language values not correct?
Comment #88
BerdirFixed some more translation fails and fixed the translation web test.
Comment #90
plachThe attached interdiff should fix the latest field translation test failures. However to properly fix them we'd need #1810370: Entity Translation API improvements to go in first, as right now the Entity Translation API does not support multilingual field defaults.
Moreover #2019055: Switch from field-level language fallback to entity-level language fallback is going to completely change how language fallback is applied to entities, so the field display language tests are going to be completely rewritten (or probably just thrown away).
Comment #91
BerdirAwesome, thanks!
Here's an updated patch with a major re-roll after all the field array/object changes in the test and also some test fixes.
The tests right now check for the following three things:
1. A non-existing field, is not touched.
2. A field that is set to NULL, values are deleted
3. A field that is an empty array(), values are deleted.
With the check in EntityBCDecorator, I fixed case 2 and 3 but this causes the first to blow up. Does this case even exist anymore, with the defined fields, you can't not have it, can you? So should we simply delete those?
Comment #93
BerdirFixed some wrong merges and trying to fix sql storage test.
Comment #95
BerdirOk, that __isset() change does weird things.
Removing that, let's try something else, I still think that we no longer support this like that.
Comment #97
Berdir#95: convert-test-entity-to-entity-test-1822000-95.patch queued for re-testing.
Comment #99
plachBtw, I really think we should try to make #1810370: Entity Translation API improvements go in asap. It will make completing the field NG transition easier and shouldn't conflict much with the other patches.
Comment #100
Berdir6 fails!
Need to look at why the field attach other suddenly fails, it didn't do that before. Taxonomy term index is also weird. Other than those, the remaining ones is the handling of missing fields/incomplete entity objects, which I think we no longer support? And one last translation related fail in field sql storage.
@plach: Yes, following there, will do a review ASAP. Maybe you could have a look at the last translation related fail here in the field sql storage test? I'm not sure if all my changes there make sense...
Comment #101
plachI will later today :)
Comment #102
fagoIndeed - we don't.
Comment #103
plachI am looking at the storage failures
Comment #104
plachThe attached interdiff fixes the field attach storage tests: actually one use case is not supported anymore (fields non defined on the entity object). In the other fix I just realigned the code with original one, which seems broken however. We probably may want to change the assertion and keep the unset instead.
Comment #105
BerdirUps, miscommunication, I fixed the same failures on the plane. My patch is basically doing the same as plach's.
- Completely remove the missing-check from field_attach_update(), that whole thing is going out anyway and we're already in a foreach loop on the field instances there, I was just playing around with that check
- Fix the caching test
- Removed the missing field tests, as discussed, they are no longer supporter, especially after we move to methods on field classes.
- Fix the TermIndexTest, that also did unset() crazyness to avoid re-loading the node o.0.
If those changes don't introduce new failures, I think we're left with ***1*** fail here, "NULL field translation is wiped." in FieldSqlStorageTest, which is the one that I meant in #100.
Comment #106
BerdirTagging.
Comment #108
plachThis should be green.
Comment #110
BerdirQuite a re-roll after the field types issue went, also added a few missing files that got lost in the last patch.
A few test methods were removed that we had to fix here, so that patch is a bit smaller, let's see if this works.
Comment #112
BerdirWho thought it would be a good idea to switch the arguments of that function?! ;)
Comment #114
Berdir#112: convert-test-entity-to-entity-test-1822000-112.patch queued for re-testing.
Comment #116
alexpott#112: convert-test-entity-to-entity-test-1822000-112.patch queued for re-testing.
Comment #118
Berdir#112: convert-test-entity-to-entity-test-1822000-112.patch queued for re-testing.
Comment #119
effulgentsia CreditAttribution: effulgentsia commentedThis looks great. Just some minor questions.
Should we rename the method as well?
And this one.
Why?
Since we're changing this anyway, shouldn't it be $entity_type and $bundle?
This seems wrong. Aren't 'eid', 'evid', and 'fttype' no longer the correct key names? Is it a problem that the test isn't failing with these being incorrect?
Comment #120
effulgentsia CreditAttribution: effulgentsia commentedThe feedback in #119 is really minor. Once it's addressed, please feel free to RTBC without waiting for me (since I'm 8 hours behind Dublin time).
Comment #121
fagoThe patch looks good to me. So leaving needs-review so that the concerns of #119 can be addressed.
I'm wondering what should happen with these tests in the long run considering #2028403: Mark field_attach_* functions as deprecated (which I just opened). I guess we should either a) remove them if we have already an entity equivalent or b) port it over to the entity system to ensure the same thing is covered there + decide on a test-by-test basis.
Comment #122
plachYes, I already expressed similar concerns to @Berdir. Some tests were already just dropped here just to make the bot happy as we don't support the related use cases anymore.
Comment #123
BerdirThanks, addressed that, also found a left-over form controller to kill.
- Renamed the functions to Empty instead of Missing
- Fixed the bundle, there was only a single case that uses it and that passed in the entity type name. Changed the default value to NULL as the default bundle == passed in entity type.
- Fixed the eid/evid/fttype thing. I don't think it's a problem that it didn't fail, if it's not set then it just uses the default values.
Also just noticed a some unchanged import thats that still references the old entity type but didn't break but it just verifies that the raw files are copied over.
Comment #125
Berdir#123: convert-test-entity-to-entity-test-1822000-121.patch queued for re-testing.
Comment #126
plach#119 has been properly addressed and both @fago and @effulgentsia agree this is good. Let's make another step towards removing the Entity BC layer!
Comment #127
plachComment #128
alexpottCommitted d0d1817 and pushed to 8.x. Thanks!
Comment #129
plachWho-hoo!
Comment #130
BerdirYay!
Wrote a short change notice for this, it was quite common to use the old test_entity's for tests: https://drupal.org/node/2028829
Comment #132
jibranWe have a remaining @todo in BlockContentFieldTest
Comment #133
pcambraAsked Berdir about this and completely forgot to create the issue, here it is: #2428633: Remove unnecessary BlockContentFieldTest