Skip to content
Snippets Groups Projects

Resolve #3494273 "Move conversion test cases"

Merged Resolve #3494273 "Move conversion test cases"
issue/experience_builder-3494273:3494273-move-conversion-test into 0.x
4 unresolved threads
Merged Ted Bowman requested to merge issue/experience_builder-3494273:3494273-move-conversion-test into 0.x
4 unresolved threads

Closes #3494273

Merge request reports

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
tests/src/Functional/ApiContentUpdateForDemoControllerTest.php
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
    Wim Leers
    Wim Leers @wimleers ·
    Maintainer

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

  • Ted Bowman
    Ted Bowman @tedbow ·
    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
  • Lee Rowlands
    Lee Rowlands @larowlan ·
    Maintainer

    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?

  • Ted Bowman
    Ted Bowman @tedbow ·
    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.

  • Wim Leers
    Wim Leers @wimleers ·
    Maintainer

    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
  • Wim Leers
    Wim Leers @wimleers started a thread on the diff
    • tests/src/Functional/ApiContentUpdateForDemoControllerTest.php
    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, [], []);
  • Wim Leers
    Wim Leers @wimleers started a thread on the diff
    • tests/src/Functional/ApiContentUpdateForDemoControllerTest.php
    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
  • Wim Leers
    Wim Leers @wimleers started a thread on the diff
    • tests/src/Functional/ApiContentUpdateForDemoControllerTest.php
    73 73 ],
    74 74 ],
    75 75 ]
    76 76 );
  • Wim Leers requested changes

    requested changes

  • Wim Leers assigned to @tedbow

    assigned to @tedbow

  • Wim Leers requested review from @larowlan and @wimleers

    requested review from @larowlan and @wimleers

  • Wim Leers
    Wim Leers @wimleers ·
    Maintainer

    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

    Toggle commit list
  • Wim Leers merged

    merged

    • Please register or sign in to reply