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 Workflow 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 |
---|---|---|---|
#16 | 2843785-16.patch | 8.01 KB | Sam152 |
#16 | interdiff.txt | 2.07 KB | Sam152 |
#11 | interdiff-7-11.txt | 7.1 KB | Anonymous (not verified) |
#11 | 2843785-11.patch | 7.92 KB | Anonymous (not verified) |
Comments
Comment #3
timmillwoodModerationState entity doesn't exist anymore.
Comment #4
timmillwoodComment #5
Wim LeersThanks for updating this!
Comment #6
Wim LeersComment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #10
Wim LeersLet's add this test coverage only to 8.4.x, for the same reason as #2843770-11: EntityResource: Provide comprehensive test coverage for ResponsiveImageStyle entity:
Review:
The Workflow module is experimental.
Therefore these belong in
core/modules/workflows/workflows.link_relation_types.yml
.The Workflow module is experimental.
Therefore these belong in
core/modules/workflows/tests/src/Functional/Rest
.So: move a few things, then this is ready :) Thanks!
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commented@Wim Leers, thanks for the tip! Done.
Comment #12
Wim LeersThanks!
Comment #13
larowlanAre these correct after the move?
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedThese correct for me, because @group allow to check all rest tests regardless of the location.
Comment #15
Wim Leers@larowlan They're correct (they're consistent with others), but they actually should also have
@group workflows
is what you're saying. But in Drupal core, we apparently don't do multiple@group
s anymore. There's no clear rules for this.So, leaving it to you to decide what makes sense to you: either keep it as-is, or change all of those to
@group workflows
.Comment #16
Sam152 CreditAttribution: Sam152 commentedWe have test workflow type plugins we can use, so we aren't coupling workflows tests to content_moderation. Using them makes sense to me in this context.
Comment #17
Wim LeersOh, even better!Actually, this is not just about test coverage. These functional tests are designed to mimic what actual developers in the real world do, what they expect to work, and guarantees that a given mistake results in a given helpful error message.
Because of that, I think I actually would prefer to use the "real" thing.
I don't feel strongly about this though.
Comment #18
Sam152 CreditAttribution: Sam152 commentedWe'd want any workflow types someone might implement to also work and be covered. It's closer to testing the abstraction vs an implementation, but I don't feel that strongly about it either.
Content moderation is probably on the extreme side of how much is wrapped around the specific workflow type, but you could argue it should introduce its own test for those edge cases, if they exist, and workflows should just be testing the abstraction and functionality it provides.
Comment #19
Wim Leers#18: Works for me :)
Back to RTBC. Still up to @larowlan to make a choice wrt #15.
Comment #20
Wim LeersThis is actually a must-have for the Workflows module to reach beta-level stability, because this helps prevent BC breaks.
Comment #21
Sam152 CreditAttribution: Sam152 commentedAdded this to the "must-have" in the roadmap #2843494: WI: Workflows module roadmap.
Comment #22
Wim Leers#21: ❤️
Comment #24
larowlanUpdating credit to include Wim who provided reviews and mentoring here.
Can we get a follow up to move the tests back into rest module when workflow module is stable as per #10?
Committed as 6002539 and pushed to 8.4.x. Thanks!
Gotta love those all numeric shas.
Comment #25
larowlanComment #26
Wim LeersHm, actually, isn't it better like this? I'd love to eventually move all this REST test coverage into the respective modules at some point I think.
(It means everything is self-contained — imagine each core module being exposed as a separate composer package.)
Comment #27
larowlanRoger that
Comment #28
Wim LeersI'd have been happy to open a follow-up if you disagreed — but if you agree, that's even better of course!