Overview

in FieldUninstallValidatorTest we have a todo to add extra test coverage

Proposed resolution

Add as many of these test cases as we can. To be practical we don't need to do all the test cases in 1 MR we can commit it case as we finish it.

  1. - Field is used in previous revision.
  2. Default value stores the expression unescaped Very unsure if we need to support this so don't need a test right now
  3. Field used in multiple entities of different types.

    We need a test where an experience builder field is used in another content entity type, such as user or comment, in addition to nodes.

    The expected uninstall error should show the messages for both entity types for both default values and content uses.

    The experience builder field for the test will either need to be created programmatically in the test or the xb_test_config_node_article test module could be updated to provide the field on the another content type also(the module name would need to be updated too.

    It is probably better if possible to create the field programmatically so we don't have to keep updating the test modules when we need different fields.

  4. Low priority: Test with field that does not use dedicated storage.

    See

    // Check whether the field has dedicated storage.
          if ($table_mapping->requiresDedicatedTableStorage($component_field_storage)) {
            $base_table = $table_mapping->getDedicatedDataTableName($component_field_storage);
            $revision_table = $table_mapping->getDedicatedRevisionTableName($component_field_storage);
          }
          else {
            $base_table = $table_mapping->getBaseTable();
            $revision_table = $table_mapping->getRevisionTable();
          }
    

    We only test one of these, I believe most fields fall into the 2nd case but debugging would need to confirm which case our test covers.

    Is the uncovered case common? Is there a test field we can use in a core test module or test that creates the other case that we don't test?

Any others?

User interface changes

None

CommentFileSizeAuthor
#5 revision-ids.png97.42 KBakhil babu
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

tedbow created an issue. See original summary.

akhil babu’s picture

Assigned: Unassigned » akhil babu

akhil babu’s picture

I've added a test for validating the previous revisions. The test creates 3 revisions for the node. The link prop is used in the second revision. So, the exception message should contain '2' as the revision id.
The test works fine in local. But for some reason, revision id is coming as '1' in the exception message in gitlab ci and the test is failing...

akhil babu’s picture

akhil babu’s picture

Assigned: akhil babu » Unassigned

Unassigning for now

tedbow’s picture

Status: Active » Needs work

Setting to needs work since there is existing Merge Request.

@akhil babu Thanks for getting this started!

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

deepakkm’s picture

Assigned: Unassigned » deepakkm

Picking this up and assigning to myself

tedbow’s picture

Assigned: deepakkm » tedbow

@deepakkm very close! Just a couple small things. I will fix those so we can get this MR merged quickly. Thanks!

tedbow’s picture

Assigned: tedbow » Unassigned
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

@deepakkm this looks good for the "Field is used in previous revision." case. Thanks!

I will merge if the tests pass

wim leers’s picture

Status: Reviewed & tested by the community » Needs work

Only one of the 4 in the @todos in the codebase and in the issue summary is done? 😅

deepakkm’s picture

@tedbow Thanks for reviewing , @wim yes only 2 of them is done in this but as per discussion between me and ted this will go in pieces.
Once this MR is merged I’m going to open another MR for 1 of those tests. Hope that makes sense.

And yes this shouldn’t be RTBC’ed 😜

tedbow’s picture

Issue summary: View changes

  • tedbow committed 6fd89f0d on 0.x authored by akhil babu
    Issue #3471028 by deepakkm, akhil babu, tedbow: Expand test coverage in...
tedbow’s picture

Assigned: Unassigned » deepakkm
Issue summary: View changes

@wim leers yep, sorry, I chatted with @deepakkm about this but forgot to update the summary.

I think we should commit each test case in their own MR to be practical. If we get just 1 done that is a win.

So will merge each as I review. will be clear next time which case is covered when RTBC and merge.

I just merged Field is used in previous revision.

I updated the descriptions of the other test cases

tedbow’s picture

@deepakkm I suggest you take on Field used in multiple entities of different types next

wim leers’s picture

I think we should commit each test case in their own MR to be practical. If we get just 1 done that is a win.

💯👏

Great call! 😄

deepakkm changed the visibility of the branch 3471028-Expand-test-coverage-module to hidden.

tedbow’s picture

Version: » 0.x-dev
Assigned: deepakkm » tedbow
Status: Needs work » Needs review
Issue tags: +Needs follow-up

@deepakkm one more follow-up needs to be made, see https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

I also made another one you can work on also https://www.drupal.org/project/experience_builder/issues/3476891

I think this looks good. But want to review it with fresh eyes tomorrow

tedbow’s picture

Assigned: tedbow » Unassigned
Status: Needs review » Reviewed & tested by the community

This looks good to me. Will merge if tests pass

  • tedbow committed 61ba9554 on 0.x authored by deepakkm
    Issue #3471028 by deepakkm, tedbow, akhil babu: Expand test coverage in...
deepakkm’s picture

Issue summary: View changes
quietone’s picture

Issue tags: -Needs follow-up +Needs followup

Just changing the tag. It really isn't hyphenated.

deepakkm’s picture

@tedbow - are we good to close this or do we need to work on the 4th issue i.e Test with field that does not use dedicated storage?
i know as per our discussion this was a very rare scenario and not as urgent as others.

wim leers’s picture

Assigned: Unassigned » tedbow
tedbow’s picture

@deepakkm can you make the follow-up issue I asked for here https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

After that re #28 let's mark this issue postponed on #3477428: Refactor the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values because that is might change field storage, and probably make sense to postpone writing the remaining test case till after that is decided.

tedbow’s picture

Title: Expand test coverage in FieldUninstallValidatorTest » [PP-1] Expand test coverage in FieldUninstallValidatorTest
Assigned: deepakkm » Unassigned
Status: Needs work » Postponed
Issue tags: -Needs followup

Created the follow-up #3478866: FieldTypeUninstallValidator's error message needs to include the entity type and bundle of the default value field

Postoning on #3477428: Refactor the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values since that might change on things are stored. This issue is still minor Priority. I think it got less important since we have covered 2 of the more likely cases in the tests that were already merged