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.
-
- Field is used in previous revision. -
Default value stores the expression unescapedVery unsure if we need to support this so don't need a test right now 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.
- 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
Comment | File | Size | Author |
---|
Issue fork experience_builder-3471028
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
Comment #2
akhil babuComment #4
akhil babuI'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...
Comment #5
akhil babuTried printing the revision ids and it's properly updated after each
$node->save()
.https://git.drupalcode.org/issue/experience_builder-3471028/-/jobs/2709034#L359
Comment #6
akhil babuUnassigning for now
Comment #7
tedbowSetting to needs work since there is existing Merge Request.
@akhil babu Thanks for getting this started!
Comment #9
deepakkm CreditAttribution: deepakkm at Acquia commentedPicking this up and assigning to myself
Comment #10
tedbow@deepakkm very close! Just a couple small things. I will fix those so we can get this MR merged quickly. Thanks!
Comment #11
tedbow@deepakkm this looks good for the "Field is used in previous revision." case. Thanks!
I will merge if the tests pass
Comment #12
wim leersOnly one of the 4 in the
@todo
s in the codebase and in the issue summary is done? 😅Comment #13
deepakkm CreditAttribution: deepakkm at Acquia commented@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 😜
Comment #14
tedbowComment #16
tedbow@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
Comment #17
tedbow@deepakkm I suggest you take on Field used in multiple entities of different types next
Comment #18
wim leers💯👏
Great call! 😄
Comment #22
tedbow@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
Comment #24
tedbowThis looks good to me. Will merge if tests pass
Comment #26
deepakkm CreditAttribution: deepakkm at Acquia commentedComment #27
quietone CreditAttribution: quietone at PreviousNext commentedJust changing the tag. It really isn't hyphenated.
Comment #28
deepakkm CreditAttribution: deepakkm at Acquia commented@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.
Comment #29
wim leersComment #30
tedbow@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.
Comment #31
tedbowCreated 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