See parent task.

TODO ON ME: check if tests for JSON serialization can be easily added to this module or if e.g. lupus_ce_renderer is a better place.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

roderik created an issue. See original summary.

fago’s picture

Title: Create test for checking markup serialization » Test coverage for markup and JSON serialization
Priority: Normal » Major

I second that, the parent issue also triggered that thinking for me.

We should make one test that covers all cases that are relevant for serialiation, what I can think of is:

CE with attributes, no children
CE with attributes, single slot, single CE child
CE with attributes, single slot, multiple CE children
CE with attributes, single slot, markup cotnent
CE with attributes, single slot with multiple entries, one single child, one multiple CE children. adding markup in this case is unusual, so not something we have to cover imo.

then, we should serialize this in those formats:
markup with vue2/standard CE syntax
markup with vue3 syntax
json

This is not a release blocker, but I'd love to get it done soon to ensure we are good and stay good here. We should make this one or multiple dedicated kernel tests, so we can easily add them to 2.x and 3.x and compare results.

roderik’s picture

For reference to whomever:

Markup:
See renderCustomElement() in existing tests.

$render = $customElement->toRenderArray();
$output = (string) $this->container->get('renderer')->renderPlain($render);

JSON:
$output = $this->getCustomElementNormalizer()->normalize($custom_element). (There is no place in custom_elements module that does JSON serialization, but the building blocks - i.e. getCustomElementNormalizer() are here so the test should be here. One module that has the above line of code, is lupus_ce_renderer.)

benellefimostfa made their first commit to this issue’s fork.

benellefimostfa’s picture

@roderik First iteration of the test is pushed, which is failing when having multiple slots after normalizing.

roderik’s picture

Status: Active » Needs work

This looks good, and I also learned things about CE in the process. (The outdated issue summary is a sign of how little I knew about CustomElement basics back in September -- I likely won't bother updating it.)

I have some requests for additions though; see MR comments.

Please change ticket status to "Needs review" when I should review.

Also: I will want to have a larger comment on the class (to say that it is not only testing CustomElements but also rendering) and possibly rename it. But I think I'll just do that myself afterwards, and not bother you with that.

adding markup in this case is unusual, so not something we have to cover imo.

I actually think we'll need to cover this because it will become more important. But we can add that coverage in #3494426: Create markup to custom-elements tree parser after merging this.

roderik’s picture

Reviewed. This is good. But done:

  • In https://git.drupalcode.org/project/custom_elements/-/merge_requests/109#..., when I said "single setSlot() call" and "above", I meant the test above my comment that contained a single setSlot() call. Because that would test/document an isolated addSlot() call. You have extended a more complicated test, so... I now added this myself. I respected/took the code setup that you used in other tests.
  • testSlotOperationsWithMultipleChildren() did not contain the Vue3 version yet, so I added it for consistency - and changed the test comments a bit.
  • I renamed the class, and commented that it actually tests the combination of CustomElement + CustomElementsNormalizer.

I don't usually merge issues that are still set to Needs Work, but review was requested elsewhere and we can continue in #3494426: Create markup to custom-elements tree parser if needed.

roderik’s picture

Status: Needs work » Fixed

  • roderik committed 0a475bf8 on 8.x-2.x
    Issue #3473955 by benellefimostfa, roderik, fago: Test coverage for...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.