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.
Use PHPUnit to test the Entity, ConfigEntityBase, and ContentEntityBase classes. In order to do that, add wrappers to access services and procedural code.
Comment | File | Size | Author |
---|---|---|---|
#116 | drupal_2134857_116.patch | 57.29 KB | Xano |
#73 | interdiff.txt | 7.37 KB | Xano |
#73 | drupal_2134857_73.patch | 57.64 KB | Xano |
#8 | interdiff.txt | 47.28 KB | Xano |
#5 | interdiff.txt | 10.17 KB | Xano |
Comments
Comment #1
XanoComment #2
XanoWork is still in progress, but feedback is welcome.
\Drupal\Core\Entity\Entity
is tested and I will continue work onContentEntityBase
.Comment #3
XanoThere should be exactly 50 failures.
Comment #5
XanoComment #6
XanoI would love some help with testing the methods used for/with typed data. They're sometimes complex and sometimes require magic methods and I'm not sure how to deal with that.
Comment #8
XanoI really need some help with the typed data stuff. This patch also removes a few Simpletest tests that are no longer necessary and I believe we can remove some (parts of) others as well.
Comment #10
XanoThe previous patch was incomplete.
Comment #12
XanoConfigEntityBase::sort()
needs work, because it depends on a publicweight
property, that doesn't necessarily have to be present on the entities the callback is supposed to check. We can either extendConfigEntityInterface
with weight methods now, or we can introduce aWeighted(Entity)Interface
with a trait now that core will require PHP 5.4, so config entities can optionally implements weights.Another issue is
ConfigEntityBase::preSave()
, which requires anEntityStorageControllerInterface::getQuery()
method. However, that method only exists onConfigStorageController
.Comment #13
XanoConfigEntityBase
now has full test coverage.ConfigEntityBase::sort()
only accepts array items that implementConfigEntityInterface
, partly because it requires thelabel()
method.ConfigEntityBase::preSave()
now checks if the storage controller extendsConfigStorageController
before calling the class-specificgetQuery()
method on it.ContentEntityBase
is still unfinished because of typed data.Comment #14
tim.plunkettThe title is misleading. This isn't really DI, but it is a good idea! Can you retitle it?
For all of the service retrieval methods, please use the pattern we've adopted in core:
When would this not be ConfigStorageController?
We already have String::checkPlain and String::format, no need for this
How is this in scope?
How is this in scope either?
This change contradicts the comment.
These should also have \PHPUnit_Framework_MockObject_MockObject separated by |
Comment #16
damiankloip CreditAttribution: damiankloip commentedQuick pass:
Where's the default gone?
testIsNew, but this is also using enforceIsNew?
Also, all of these calls to assertSame() don't need to do spl_object_hash(), PHPUnit will assert whether the object is the same instance or not for you.
This can just use $this->once()
This can just use one static string: 'MyClass::sortMethod'
This doesn't really test alot, and fails if we are trying to adhere to strict standards, are we? not sure :)
Do we need a variable for this?
huh?
Also, Tim is right. The issue title is totally not what is going on here :-) it also feels a bit like most of the changes are solely for making it testable in phpunit.
Comment #17
XanoDone.
Done. I know it was agreed upon that configuration entity storage is not swappable after all, but old habits die hard.
Done.
#2095125: Use access constants in every access control context removed the default argument from the interface and I saw removing it from the classes as a clean-up. It also prevents us from having to test it.
Do you have a suggestion?
taxonomy_vocabulary
?Done.
The method tests the getter and the setter. Is there another way to test these separately without using Reflection to test the property values, which really tests the internals rather than the integration?
You're right that it doesn't matter for the assert, but in case of failures, it's easier to compare hashes than
var_export()
output, and it doesn't mess up the Simpletest UI as much.Done.
Done.
What do you mean with strict standards? At the very least this test ensures that the method can be called without trouble.
Not necessarily, but I find it more descriptive than just passing on an empty array directly.
It might have been fubhy who called this wicked. All we need is a mock of an arbitrary class so we can test that the method is called. It does not matter what class the mock is of, so I chose an existing class, rather than adding a new one just for this.
Comment #18
XanoHere's the interdiff of the last patch.
Comment #20
dawehnerMissing phpunit tag.
Comment #21
dawehnerJust an idea use \Drupal\Component\Utility\String::format()
+1 to sync the signature.
Let's use @group Drupal, and maybe even @group Entity or Config.
Don't we also have an interface for that?
Instead of mocking the actual tested class you can also create a TestConfigEntityBase class in the same file as the test, which "mocks" these methods in real code.
see @group Drupal
Doesn't __FUNCTION__ refer to testLabel(), this code is damn complicated.
Comment #22
BerdirAs discussed in IRC, I think it's perfectly fine to ignore the Fields/TypedData stuff in ContentEntityBase or actually almost everything in that class (since it's almost completely about fields and field (list) classes). That's complicated stuff and shouldn't block this issue, but a follow-up would be great.
That said, I still think that #1867228: Make EntityTypeManager provide an entity factory is the most important issue right now in regards to all the factory/service/DIC stuff.
Comment #23
Xano17: drupal_2134857_17.patch queued for re-testing.
Comment #25
XanoRe-roll, plus I addressed some feedback, and removed some unfinished testing methods that were for typed data.
Done.
Added
@group Drupal
.Done.
Comment #27
XanoMinor changes. Uploading, because I am getting unexplained test failures locally. Maybe the testbot can give some more detailed information.
Comment #29
XanoI'm still experiencing some PHPUnit errors locally that don't show up on screen or in my logs.
Comment #31
XanoComment #33
dawehnerSorry this was not a really good review to be honest, as the time is already ongoing.
We do use @return $this in those usecases now.
Why do we change the default operation?
TF?
see before
oh php (btw. "oh php" is really easy to type)
<3
What about using @group Config as well?
Where have you found that syntax? This is kinda cool
@group Config
Maybe we could swap this two lines.
@group Entity + I wanna use the "{$testedClass}Test" pattern as it allows you to switch with one key combination (ctrl shift t for me) between test and implementation. Beside of that handy feature this is kind of a standard already in core.
Afaik we do have an interface for that.
Lets use a newline at the end.
Comment #34
Xano@dawehner: both quotes are yours and from this issue ;-)
Which is short for...?
It's in the official PHPUnit documentation, but timplunkett made me aware of it.
Comment #36
XanoComment #38
tim.plunkettI'm pretty sure TF is the last two letters of WTF.
This isn't really testing *anything*. It seems like it's just here for code coverage percentage reasons?
Comment #39
XanoEven then I still don't know what the problem is. Please elaborate.
It does test something, but I agree it's not much. All the interface says the method must do is Returns a string representation of the data., so I'm not sure there is much else we can test without getting very implementation-specific. Feedback and ideas are always welcome, though :)
Comment #40
tim.plunkettYou have \PHPUnit_Framework_MockObject_MockObject in core/lib/Drupal/Core/Entity/Entity.php.
Comment #41
XanoWell, that's an embarrassing error...
Comment #43
XanoComment #46
Xano43: drupal_2134857_43.patch queued for re-testing.
Comment #48
XanoRe-roll because
$entity_info
was renamed to$entity_type
in a few places.Comment #50
quietone CreditAttribution: quietone commentedI think has been fixed in commit 878777a842d8c12545ce5152c8e117798a7ab4dd
Can someone confirm and close this issue
Comment #53
XanoWell-spotted! However, the patch from #48 was faulty, as you can see by comparing the file size and contents to the patch from #43. I must have accidentally messed up when I created the re-roll.
Here is a proper re-roll from #43 that does contain the correct code and applies to HEAD.
Comment #55
XanoAdded DependencySerialization to entities.
Comment #57
XanoComment #58
dawehnerWhat are the reasons for those changes?
Comment #59
XanoYou asked this before in #33 and I answered the question in #17 and #34. Please know that this is fine, but I just want to illustrate that more people noticed this change and I have not simply ignored questions about it).
You already agreed on this change in #21:
But in short, the reason I changed this is that the interface no longer has a default operation and as the PHPUnit test tests the class's interface implementations, I wanted the code to reflect this.
Comment #60
dawehnersorry for that.
Comment #61
alexpottNeeds a reroll
Comment #62
XanoRe-roll.
RTBC, as this was the only reject, because the change was already made in another issue:
Comment #63
XanoComment #64
XanoComment #66
Xano62: drupal_2134857_62.patch queued for re-testing.
Comment #68
XanoRe-roll.
Comment #70
XanoComment #71
dawehnerBack to rtbc
Comment #72
alexpottMy main concern is that not all the tests appear to be testing that much.
This would lead to a false sense of coverage. We should be testing the implementation details of getExportProperties here - how about asserting that is does not contain private /protected properties (isSyncing for example) and contains expected public properties - for example uuid or status.
This does not seem to be testing the functionality of getString()
Should we not test that onSaveOrDelete is called? And should we not be testing the logic in the onSaveOrDelete method here? i.e asserting the resetCache method is called on the viewbuilder for referenced entities?
Comment #73
XanoI addressed the feedback from #72.
Comment #74
damiankloip CreditAttribution: damiankloip commentedThose changes looked like they addressed concerns of not really testing anything :)
Comment #75
alexpottCommitted 236a40b and pushed to 8.x. Thanks!
Huh? Not reason to do this. Fixed in commit
Comment #76
damiankloip CreditAttribution: damiankloip commentedXano, git core.filemode setting could be your friend :)
Comment #77
XanoAwesome, thanks @alexpott!
@damiankloip: Will look into that. I probably did a bad job at resetting Git after re-installing core. Thanks :)
Comment #78
webchickUnfortunately, this patch caused random testbot failures in Toolbar module of all the silly things, see #2216701: Random test failure in ToolbarAdminMenuTest for an epic sleuthing adventure. The patch there rolls this one back, so back to "needs work."
Comment #79
alexpottI think we should replace language_load() with a call to
\Drupal::languageManager()->getLanguage($langcode);
and we should mock the language manager. Then we can use PHPUnit's getMockForAbstract which means we can test all concrete implementations in these classes.This should be $entityType
Similarly this should be $entityTypeId
I have no idea why this caused the segmentation faults though - but my guess is something to do with the serialization code. Perhaps in the service wrappers lets not store a reference to the object on the class.
Comment #80
alexpottIt seems this patch also might have caused #2216909: Random test failure in ConfigTranslationUiTest
Comment #81
xjmPer @Wim Leers, #2217749: Entity base class should provide standardized cache tags and built-in invalidation is currently blocked on this, and that issue is currently filed as a beta blocker. It might be more of a beta target, but let's consider this one at least a major for that reason.
Comment #82
BerdirSo here's a an update to get started, addressed the review from above. Let's see if that still causes random fails in the toolbar test. (uploading a toolbar test for that, with the hacks from https://drupal.org/node/2216701 to just test that class).
That said, I'm not so sure about certain parts here, I'd prefer not adding fake test coverage for methods when we know that w'ere not properly testing them (some special cases not being tested is fine, I mean stuff like testIsNew() and testUuid().
Comment #83
Berdir82: drupal_2134857-82-toolbar.patch queued for re-testing.
Comment #85
Wim LeersRe-uploading the test patch in #82, just to see if we get the same or similar results. Because at least for me, the failed tests output in #82 makes no sense.
Comment #86
Wim LeersThey came back green. That's 40 times no test failure. It would almost seem like #82 suffered from a random testbot problem. Let's see if we can get another 40 test runs that are green. In which case I think it's safe to assume that #82 indeed contains the solution.
Comment #87
Wim Leers85: drupal_2134857-82-toolbar.patch queued for re-testing.
Comment #88
Wim Leers85: drupal_2134857-82-toolbar.patch queued for re-testing.
Comment #89
XanoIsn't this redundant? Mocked methods return NULL anyway, and the expectation does not need to be set.
Same here.
And here.
We should also add setters instead of setting a custom container in the test, but we may want to fix the failures before we address this particular issue.
Comment #90
alexpott@Xano
Why?
Comment #91
tim.plunkett->will($this->returnValue(NULL));
is redundant when the matcher is $this->any(), which is true in some places in the patch.$this->once(), $this->atLeastOnce(), or $this->exactly($count) might be better choices.
There are some cases it's used with $this->at($index), which is good
Comment #92
BerdirYep, and some of those cases are IMHO actually wrong. getLanguage() for example should verify that a valid language is passed in and should return the corresponding language object, right now, it returns NULL and then falls back internally to not specified, which is not something that should happen during actual runtime.
Comment #93
XanoBecause during tests we want to isolate classes as much as we can. If we use getters like these, we should also add setters, or they won't be useful at all. There is no way to make those protected properties contain the required services now.
Comment #94
Wim LeersSo, #85 came back fully green again. That means a total of 80 test runs without a failure. Which implies #82 contains the solution. And I think we can address Xano's concern and restore Xano's code and still have it working, actually. (Local tests seem to indicate this at least.)
(While working on this, I was slowed down massively by #2216695: run-tests.sh broken on OS X ("Too many open files").)
Comment #95
BerdirHm, that is exactly the change that I made above to make it work. And yes, it works locally for all of use with and without that :)
My patch is just as capable as Xano's was before, if $this->entityManager exists, then it is used, the only difference is that I don't store a reference to it. There were no setters before, so I don't understand his comments as he didn't add them either ;)
Comment #96
Wim Leers#95: I'm merely building upon what you did! I think Xano's concern is mainly that we'd always be returning things that live on the container — my tiny contribution here fixes that. I agree everything is just as capable as before. I'm merely cleaning up the temporary code you had in there, by respecting the original code pattern and storing a reference after all.
If you're saying you don't want those references to be stored, that's fine by me too — I don't care. If we do that though, then we must not ever pretend to store any references, and your code was still pretending that.
So that just leaves #89 to be addressed then.
Comment #98
Wim LeersRandom fail in
Drupal\image\Tests\ImageAdminStylesTest
, retesting.Comment #99
Wim Leers94: drupal_2134857-94.patch queued for re-testing.
Comment #101
webchickHm. Maybe not so random? Same fail again.
Comment #102
BerdirYes, this is one of the random fails this causes, see #80. It's in a different class this time but it's that error.
Again, that *exact* change that you reverted is what made it work, it's not cleanup. And as I said, it is exactly as flexible as the patch before, keeping a local reference does not change anything, if one is set, then it uses that. And I do agree with @Xano: that either a setter to set those properties for the unit test or mocking those methods (but then you don't need $this->entityManager at all) would be cleaner than going through the container, then we don't need those methods...
But again, that's how it was before :)
Comment #103
jessebeach CreditAttribution: jessebeach commentedThis blocks a beta blocker, so it is a beta blocker.
Comment #104
BerdirSee #81, this currently blocks that issue because it has unit tests that were written on top of this. It's more convenient to wait for this instead of rewriting them or changing them to a different type of test, but if we'd have to, we could, so I don't think it should be a hard dependency/this be a beta blocker.
Comment #105
damiankloip CreditAttribution: damiankloip commentedSorry but I still don't think this looking better in the simpletest UI is a good reason to use this over PHPUnit handling the object comparison. We adopt a decent unit testing framework like this so we can utilise its functionality, no? :)
Comment #106
Wim LeersAlright, so: #82 worked, #94 is a failed clean-up attempt (causes test failures), so this goes back to doing exactly the same as #82 but cleans it up.
Also addressed #105.
Comment #109
Wim LeersHEAD had changed in a single line.
Comment #110
damiankloip CreditAttribution: damiankloip commentedI think everything has been fixed and any concerns addressed. This is holding up some other stuff, so let's do this!
Comment #111
damiankloip CreditAttribution: damiankloip commentedSorry, after #2219925: Rename ConfigEntityInterface::getExportProperties() to toArray() got in, the ConfigEntityBaseTest needs to be updated to toArray(), minor change (missed that in the logs until now).
Comment #112
Wim LeersBack to RTBC per #110. #111 was just for chasing HEAD.
Comment #113
BerdirSorry, but this has some actual bugs (see first item) and still tons of bogus test methods.
I'm not saying that we need to implement all of them. but I'm against adding bogus test coverage so that it looks like we're covering those methods but we're not actually verying anything useful. Just dropping those that would be too much work would be fine with. I'd even be fine with just getting the necessary changes to the entity classes in as that would already unblock Wim's issue.
I think this method was removed?
$this->languageManager is no longer used and uuidGenerator should then follow the same pattern?
This overlaps with #2157467: Fix type hinting for base entity interfaces, wouldn't be needed anymore then? As it's a non-functional change, leave it for that issue to change?
I just moved the mock definition but as mentioned above, this is IMHO wrong.
This is not something that should happen. There's some strange fallback code in Entity::language() but that's not something that should really happen and we cleaned up most of what needed that when moving to content entities, which don't support that hack. This should really return an actual language object and verify the passed arguments, which should be the same as the langcode passed to the entity.
When you look at how this works, then you can see that setOriginalId() isn't really called from the outside, the more interesting part is initializing an entity object with id and then changing the id and verifying that getOriginalId() is correct. Or is that not possible as it's an abstract class?
This method is btw also moved to EntityInterface in #2218033: Move getOriginalId()/setOriginalId() to EntityInterface.
This isn't useful, createDuplicate() must result in a new UUID that is different from the original one. So the original entity should already have a UUID and the duplicate a new one. the uuid service must be mocked accordingly, otherwise we're not providing useful test coverage here.
Stale comment.
Same here, we should be working with an actual language object, otherwise we can't test that things are actually working.
This might be easier to understand when the second expects is after the first call, to see that they belong together() ? note that this would affect the at(1). Alternatively, a comment to explain it?
Comment is misleading imho, should simply say "Verify that the old value is returned." ?
That's kind of weird :) Note that #2182239: Improve ContentEntityBase::id() for better DX will allow to write a more useful test.
Most of the complex data interface methods are empty dummy implementations and will go away in #2002138: Use an adapter for supporting typed data on ContentEntities, testing them is kind of pointless :)
Why do we need to use something like __FUNCTION__ here? This is IMHO very misleading and seems to indicate that this test method is called? but it just uses the same string as this method name for the callback. What about giving it a name like "exampleLabelCallback"
Same here :)
$values = array('id' => $this->randomName()
Then you you have something that you can actually test? Same for testUuid() and testIsNew() which in the default implementation is based on having an ID.
this should then be used in combination with actually having an id(). isNew() should then return FALSE, after calling enforceIsNew(), it should return TRUE. Testing those methods on their own is useless, the only gain is that we can claim that it's covered, but that's a lie :)
Can't you compare this with $this->entityType? should be the same object.
Same here. $values should have langcode => 'en', or so, the language manager should have a corresponding with() as mentioned above and this should then verify that the correct language object is returned.
Comment #114
BerdirImplemented my review except:
12. Most of the methods are correctly tested for now, will removed them in the adapter issue.
13. Got this now. Crazy, but I guess can't be easier.
Now I'm ok with this ;)
Comment #115
BerdirComment #116
XanoRe-roll because of block.module.
Even if they aren't, we agreed to not cover typed data methods in this issue, so this should be good.
Crazy, wicked, and it saved us from having to create a dummy class with a dummy method instead. Glad this is sorted out :)
Comment #117
Wim LeersAlright! Berdir thinks this is good to go now, Xano just chased HEAD and improved a comment (the one for Berdir's 13th point).
I only changed a few LoC to get the patch good & green again, so it should be fine for me to re-RTBC this patch as per the RTBCs at #110, #114 and the earlier commit.
Comment #118
alexpottCommitted 1370edf and pushed to 8.x. Thanks!
Comment #120
xjmIn the end this one wasn't actually a beta blocker.
Comment #121
Wim LeersBut then it at least was a beta target.