Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Now we have test coverage for the very simplest case which is a single, self-contained configuration value: site slogan.
However, there are many more complicated cases than that.
Suggestion: expand this test to also include the following scenario:
- Create content type.
- Create a field on the content type.
- Deploy.
- Assert that content type and field are found.
- Delete the field and re-create it.
- Deploy.
- Other edge cases?
Comment | File | Size | Author |
---|---|---|---|
#47 | import-export-2108813-47.patch | 4.07 KB | Désiré |
#35 | import-export-2108813-35.patch | 3.58 KB | cilefen |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedwe need to cover installing/uninstalling modules, but i'll add that over in #1808248: Add a separate module install/uninstall step to the config import process.
which could use some reviews, hint hint.
Comment #2
alexpottComment #3
cilefen CreditAttribution: cilefen commentedThis is the content type only. If this looks good, somebody let me know and I will add the field.
Comment #4
cilefen CreditAttribution: cilefen commentedThis patch, adds a field and attaches it to the content type, however $web_user gets a permission denied trying to access node/{type}/add after importing the configuration and I don't know why.
Comment #6
cilefen CreditAttribution: cilefen commentedComment #7
cilefen CreditAttribution: cilefen commentedComment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedyay for this patch.
however, the assumptions it makes are not quite right.
except for properties set on $this, nothing persists between testExport() and testImport(). so, instead of deleting here, assert that the content type and field don't exist before the import.
Comment #9
cilefen CreditAttribution: cilefen commentedDone. Is it actually necessary to verify the content type and field do not exist before import?
Comment #10
cilefen CreditAttribution: cilefen commentedComment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedre #9, yes, i think asserting they don't exist is useful.
Comment #12
mtiftLooks good to me. Tests complete locally. Minor nitpick: the attached patch just cleans up 14 lines that introduced whitespace errors.
Comment #13
cilefen CreditAttribution: cilefen commented@mtift I'm usually a stickler for that kind of thing. Thanks.
Comment #14
webchickGreat work, cilefen! :D This patch makes me very happy.
I'd be fine committing this patch as-is, but one of the things I was going for with these expanded tests was some of the edge cases with fields that we keep inventing different workarounds for in CMI. That's why the issue summary said to delete the field after creating it and test that scenario as well. Any way to expand the tests to cover that, or should we just go with this and do so in a follow-up? (I'm fine either way.)
Comment #15
cilefen CreditAttribution: cilefen commented@webchick I don't understand Drupal CMI enough to know what "deploy" means in the issue summary. Can you point me to some info?
Comment #16
webchickSorry, when I say "deploy" that's short-hand for "call the doImport() function" :)
Comment #17
cilefen CreditAttribution: cilefen commentedThis patch attempts to do the events in the order asked-for in the issue summary. It throws an exception on the final doImport:
I don't know whether this is a problem with what I've done (totally possible), or an issue with CMI.
Comment #19
cilefen CreditAttribution: cilefen commentedLet's try that again. See comment #17.
Comment #21
webchickTestbot. :\
Comment #22
cilefen CreditAttribution: cilefen commentedIt fails for me, too, but I don't know why.
Comment #23
cilefen CreditAttribution: cilefen commentedI just re-ran the test on my laptop and the problem is the same--after deleting the field, then re-adding the field, and on the last import attempt, this error is returned to the browser:
Drupal\field\FieldException: Attempt to create an instance of unknown field 179b515b-f2bb-4cc0-9000-c2732cfd5715 in Drupal\field\Entity\FieldInstance->__construct() (line 254 of /Applications/MAMP/htdocs/drupal8/core/modules/field/lib/Drupal/field/Entity/FieldInstance.php).
Comment #24
cilefen CreditAttribution: cilefen commentedI ran the steps manually and I get the same error in the same place.
Comment #25
mtiftReroll because of #2133325: Create a site UUID on install and only allow config sync between sites with the same UUID
Comment #26
BerdirI don't think we need to test here that drupalCreateContentType() works. the (bool) is also a bit strange, if anything, I'd do an assertTrue(!empty($type)), but I'd just leave that out.
by reference (&) is not necessary when working with objects. Especially as you're not changing anything. Also missing spaces between if/foreach and (.
Not surprised this isn't working, that's a scary case ;) We should probably get @yched in here. I'm not even sure what it should try to do exactly, but my guess would be that it's some sort of conflict/order problem when those fields and instances are created/deleted. Just for pinning down the exact problem, does it work if create the new field with a different name?
Comment #27
cilefen CreditAttribution: cilefen commentedComment #30
webchickHeh. Ok, let's go back to the first working patch then, and cover this expanded test case in a follow-up. The first patch does a good job of laying groundwork so the next time we jigger UUIDs or what have you we can add test coverage for the actual bug we're trying to fix.
So let's get a re-roll of #12 w/ the feedback from Berdir in #26 incorporated.
Comment #31
mtiftThis patch should address the comments in #26 and #30. All of the tests work locally.
Comment #32
star-szrTagging for reroll.
Comment #33
InternetDevels CreditAttribution: InternetDevels commentedReroll. Removed unused variable $admin_role and added some doc to getInfo() and setUp().
Comment #34
star-szrThanks @InternetDevels, we lost a bunch of assertions from what I can see so this needs another look.
Comment #35
cilefen CreditAttribution: cilefen commentedThis is essentially what we were trying in #12 with the suggestions from #26 included.
Comment #36
xjmThe test has been completely rewritten, so we need to just add the new test data and assertions into it.
Comment #37
xjmOops, crosspost. #35 has more coverage so go with that. :)
Comment #38
tim.plunkettIs #2108809: Make config import/export tests pass with standard profile a truly separate issue and critical in its own right?
Comment #39
alexpottChanging to beta target since this is not actually going to make API changes :)
Comment #40
mtift35: import-export-2108813-35.patch queued for re-testing.
Comment #41
mtiftPatch from #35 applies cleanly. Code looks good. Removes a few unused variables. Covers each of the items in the issue summary. Test complete locally. If testbot agrees, I'd say this is good to go.
Comment #43
alexpottNot green
Comment #44
martin107 CreditAttribution: martin107 commented#36 is green ... Its not easy being green.
Comment #45
alexpottYes but according to #41 the rtbc was for the patch in #35 and #35 has more test coverage than #36.
Comment #46
Désiré CreditAttribution: Désiré commentedComment #47
Désiré CreditAttribution: Désiré commentedI have rerolled the patch #35, fixed the warnings, and added documentation.
Comment #48
jessebeach CreditAttribution: jessebeach commented#47 has the expanded test coverage from #35.
The tests cover the use cases outlined in the issue summary by webchick, namely:
The warnings have been addressed by enabling the
node
andfield
modules in the test setup. We're good to go here with this patch.Comment #49
webchickExcellent work, all!! This is great, and something we can continue to build from for fancier edge-cases as we find them.
Committed and pushed to 8.x. Thanks!