Skip to content
Snippets Groups Projects

Resolve #3494273 "Move conversion test cases"

4 unresolved threads

Closes #3494273

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
85 $node2,
86 $invalid_image_client_json,
87 422,
88 [
89 'errors' => [
90 [
91 'detail' => "File '/not/a/real/url' not found.",
92 'source' => [
93 'pointer' => 'model.' . self::TEST_IMAGE_UUID . '.image.src',
94 ],
95 ],
96 ],
97 ]
98 );
99 // Ensure the field has not been updated.
100 $this->assertNodeXbField($node2, [], []);
  • Comment on lines -99 to -100

    :thinking: This part is not being tested anymore now?

  • Author Maintainer

    This case is moved to \Drupal\Tests\experience_builder\Kernel\ClientServerConversionTraitTest::testConvertClientToServerErrors

    That is logic we are testing. \Drupal\Tests\experience_builder\Functional\ApiContentUpdateForDemoControllerTest::testSave is still left with a test case that shows if we get an error we don't update node. It is just that we don't need to test every test case here because they are being tested elsewhere now.

    I think if we had controller for saving a node we would not feel the need to test every node validation test case in that test controller as long it was tested somewhere else.

    also since tests/src/Functional/ApiContentUpdateForDemoControllerTest.php if for a "demo" class that is going away it better if it is tested elsewhere.

    Edited by Ted Bowman
  • Is there any harm in setting these invalid values in the auto-save store (via the PHP API, not via a HTTP request) and retaining the HTTP requests and assertions here?

  • Author Maintainer

    @larowlan meaning using the auto-save store once !473 (merged) is in? An duplicating the cases in ClientServerConversionTraitTest and ClientDataToEntityConverterTest or just not moving them?

    Not a real harm doing it just duplicate test cases. Also think as we test cases in !474 (merged) which will add metadata edge case test case ClientDataToEntityConverterTest(hopefully not to controller tests) then it is going start to get confusing why some cases are in ClientDataToEntityConverterTest and some are ApiContentUpdateForDemoControllerTest and maybe some are duplicated?

    if you looked at the codebase fresh I don't think there would be clear reason why. Unless we put a comment in saying "these tests case are in ApiContentUpdateForDemoControllerTest and not ClientDataToEntityConverterTest because we addded them here first", which would seem weird to me.

  • I don't want to block this if this MR unblocks other work. But my thinking matches @larowlan's: this makes the tests harder to understand, not easier. But for the sake of avoiding duplication, which I totally get!

    I'll let @larowlan have the final say on this one, so: approving :blush:

  • Please register or sign in to reply
  • 107 $node2,
    108 $unreferenced_file_client_json,
    109 422,
    110 [
    111 'errors' => [
    112 [
    113 'detail' => "No media entity found that uses file '$unreferenced_src'.",
    114 'source' => [
    115 'pointer' => 'model.' . self::TEST_IMAGE_UUID . '.image.src',
    116 ],
    117 ],
    118 ],
    119 ]
    120 );
    121 // Ensure the field has not been updated.
    122 $this->assertNodeXbField($node2, [], []);
  • 129 $invalid_tree_client_json,
    130 422,
    131 [
    132 'errors' => [
    133 [
    134 'detail' => 'The component <em class="placeholder">sdc.experience_builder.missing_component</em> does not exist.',
    135 'source' => [
    136 'pointer' => "layout.children[1]",
    137 ],
    138 ],
    139 ],
    140 ]
    141 );
    142 // Ensure the field has not been updated.
    143 $this->assertNodeXbField($node2, [], []);
    144
  • 73 73 ],
    74 74 ],
    75 75 ]
    76 76 );
  • Wim Leers requested changes

    requested changes

  • assigned to @tedbow

  • Wim Leers requested review from @larowlan and @wimleers

    requested review from @larowlan and @wimleers

  • I'd love to see @larowlan approve this too, or suggest an alternative :fingers_crossed:

  • Wim Leers approved this merge request

    approved this merge request

  • Lee Rowlands approved this merge request

    approved this merge request

  • Wim Leers added 7 commits

    added 7 commits

    • ad4b26c2 - 1 commit from branch project:0.x
    • 222c9853 - move test cases to ClientServerConversionTraitTest.php
    • 0761a6ae - new test
    • 150ebdcc - make use of ConstraintViolationsTestTrait
    • 266abf7a - add error test for ClientDataToEntityConverterTest
    • a732f6a4 - test successful conversion
    • f02cca07 - remove unneeded comments

    Compare with previous version

  • merged

  • Please register or sign in to reply
    Loading