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.
Problem/Motivation
Please refer the problem/motivation section of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
Proposed resolution
Write EntityResourceTestBase subclass for the BlockContentType entity.
Remaining tasks
References
1. Follow-up of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method
2. Subtask of #2824572: Write EntityResourceTestBase subclasses for every other entity type.
Comment | File | Size | Author |
---|---|---|---|
#45 | interdiff-2843774-31-45.txt | 728 bytes | Anonymous (not verified) |
#45 | entityresource_provide-2843774-45.patch | 8.06 KB | Anonymous (not verified) |
Comments
Comment #3
KeeneganComment #4
KeeneganI can't make it work.
I can't even run a previous working test. For example :
returns
with a clean drupalvm install. Is it normal ?
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedThank you for wanting to help @Keenegan! You can set core/phpunit.xml.dist (copy to /sites folder for avoid changes in core files) and run from root folder like:
See also post and Running PHPUnit tests documentation.
Comment #6
KeeneganThanks for the reply !
I've tried what Wim Leers said in your post, but I have the exact same output :
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedSorry for delay with answer. Unfortunately, I have poor experience with Unix, DVM and manual run of tests (#5 from Windows console and run from PhpStorm works to me). May be interference creates by firewall, antivirus or other network settings. Perhaps launching tests will be the hardest part of this issue :)
Comment #8
Wim LeersWorks fine here:
Something must be wrong with your environment setup.
Comment #9
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedIt's possible, as ever, that I've missed something here, but when I run the functional block tests (both for the HAL module and the REST module) I get twelve passes each time:
Are we actually referring to the data available at the /entity/block_content_type/{block_content_type} endpoint?
Comment #10
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedSo the the endpoint to get the default-installed Basic Block would be /entity/block_content_type/basic?_format=json and the entity returned would look like this:
Which I'm guessing is what should be returned by the getNormalizedEntity function in a BlockContentTypeResourceTestBase class?
I'm going to assign this to myself if that's cool with everyone?
Comment #11
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedComment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedCool for me! (Although it is a pity that it failed to help @Keenegan)
#9: 12 tests = 4 tests * 3 test files - all right.
#10: I still have not won, trying to guess what will return the
getNormalizedEntity
function :) But it's not necessary to play in this game, it's much easier to sacrifice one failed test with return empty array fromgetNormalizedEntity
. And the log will contain what should be returned ;)But first we need to create
BlockContentType
entity increateEntity()
of course. The second half of the structure obtained by you in #10 will be very useful for this:Based on these data or some existing code in drupal core like
BlockContentType::create(
(lite hack!) we can create the right entity. Example:Comment #13
Wim LeersYep, thanks for taking this on, @jamesdesq! :)
Comment #14
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commented@vaplas - thanks - as always, for your help - that's really really useful.
It's been a bank holiday weekend here in the UK so I've mostly been in the pub, but I've got some time today so with any luck I'll have a first try up in a couple of hours.
Comment #15
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedOK, so one question (sorry, always with the questions!)...
I'm not sure I fully understand what you mean in #12 when you say the log will contain what should be returned.
I've got the REST tests up and running (actually 3/4 of them are redundant, since the block_content_type endpoint only implements get, but it looks as though those tests pass - I'm assuming since it's an instance of ConfigEntityInterface and so gets caught by the first condition in testPost, testPatch and testDelete).
The first part of my BlockContentTypeResourceTestBase looks like this. I've tried explicitly logging the $block_content_type variable, but nothing seems to show up in the logs. Is that what you meant, or have I misunderstood? (FTR, I also tried returning an empty array from the getExpectedNormalizedEntity, which is what I initially thought you meant).
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedOpps, sorry for bit misunderstanding with
'log'
term of #12. And do not forget that I'm not a profy, and my comments should be treated with a bit of skepticism :) Feel free to ask again.I meant the following:
And run for example
BlockContentTypeJsonAnonTest.php
. And testGet() will failed with info (log) about difference between exprected array and our empty array. It is what we need.Yes, it may seem strange way, because we use the result instead of checking the result. But it is ok now, because the system is healthy, and we simply reinforce it with additional rest tests to prevent regression in the future ;)
Comment #17
Wim Leers@jamesdesq By the way, feel free to post imperfect, in-progress patches. We all do that. It helps others to see how far along this is, and provide suggested changes to help you move quicker :)
Comment #18
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedThanks both. This is all super useful stuff to know.
So, this seems to pass locally for me. Could someone else have a try please?
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedLooks good! Mr. Bot, will you please.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedNice! Only one nit error:
Need BlockContentTypeJsonCookieTest extends... And yes, it's really pass locally, but Mr. Bot loaded all of the classes, which helped to discover it.
+ a bit cleanup:
better use
$this->entity->uuid
instead of create$uuid
.module
'block'
not necessary for testingblock_content_type
, hence$modules = ['block_content'];
.Need doc comment
@var \Drupal\block_content\BlockContentTypeInterface
.This functions can be removed, because it turned out the same as and by default.
Comment #22
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #23
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedLike this?
Also, who is Mr Bot? And how do I get him to run my tests?
Comment #24
jamesdeee CreditAttribution: jamesdeee as a volunteer and commentedComment #25
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedComment #26
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedSorry , i didn't see issue is assigned to @jamesdesq.Setting it at needs review will get Mr Bot to run tests.
Comment #27
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedForgot to interdiff, sorry...
Thanks for your patch, @gaurav.kapoor! I think yours and mine are just about the same - but it doesn't look as though you deleted the unnecessary uuid variable in the BlockContentTypeResourceTestBase.
Comment #29
Wim LeersTestbot reports one coding standards violation in its test results. Could you please fix that?
Other than that detail, this is ready!
Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commented#23 absolutely!
uuid()
is really a method (not property), good correction, thanks!#29: indeed! To get more info about testbot reports coding standards, click on the green bar under the #23 patch. Bit wait while page load, and click by 1 coding standards message (yes, cursor still default view). Screenshot.
Comment #31
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedHere it is with the coding violation corrected (I'd do an interdiff but I kind of figure for a single newline it's probably not worth it).
Thanks for the explanation, @vaplas!
Comment #32
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedComment #33
Wim Leers@jamesdesq Can you please provide an interdiff? See https://www.drupal.org/documentation/git/interdiff.
Comment #34
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedShould the interiff be between #31 and #23? Or between #31 and #18? I kind of felt like the difference between #31 and #23 was so small it wasn't really worth it, but I'm happy to do it. Or am I missing the point?
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedBetween #23 and #31. Small interdiff - nice interdiff! (regardless of its triviality). This is really useful for self-control and for simplicity of review.
Comment #36
Wim Leers#34: the point of interdiffs is to make it easier for reviewers. Right now, I have to re-read the entire patch, because I don't know which changes you made. Perhaps you changed only what was asked for, perhaps you changed more. An interdiff answers that with much more clarity: it shows exactly what changed between patches.
We all make mistakes. Interdiffs help catch those mistakes earlier :)
Comment #37
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedThose are all good reasons :)
Here's the interdiiff!
Comment #38
Wim LeersThank you!
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedOne bit remark:
we use
*Interface
(see #21.3) for descriptionentity
in all other EntityResourse tests. Maybe it is a pattern to make possible to extend (override) the class without rewriting the doc for $entity.Change status to NR for review this.
Comment #40
Wim Leers#39: hah, well-spotted! :) I'm impressed by how fast you became so proficient in rolling patches, doing reviews and doing mentoring, @vaplas!
Comment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedThank you very match for pretty words, @Wim Leers! After your adorable guidance in the adjacent issues, it would be strange to get another result. I'm very grateful to you for this, and I hope that @jamesdesq will also soon become a Jedi power!)
Change to
Needs work
to fix the type of entity.Comment #42
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedSo the final version should look like this, I think...
Once again, thanks both for all the help and encouragement on this, it's been great (though I think I'm a way of Jedi-ness just yet). I'm hoping to get some more tickets from this group done over the next few days!
Comment #43
jamesdeee CreditAttribution: jamesdeee as a volunteer and at manifesto commentedSorry, I thought I'd uploaded the interdiff but it didn't seem to work
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedThank you, @jamesdesq!
Unfortunately, the mechanism for run tests is not ideal, so in such situations we just re-upload the patch again. Like now :)
Comment #46
Wim LeersWoot! Still plenty to go in #2824572: Write EntityResourceTestBase subclasses for every other entity type. :)
This now looks ready again.
Comment #47
alexpottCommitted and pushed f170a96 to 8.4.x and 4511622 to 8.3.x. Thanks!
I've backported to 8.3.x because it is tests only.
I credited @gaurav.kapoor because they supplied a patch which for some reason d.o doesn't see. @Keenegan thanks for trying to review and contribute to the issue. I'm hope you've got phpunit set up and working now.