Overview
Having mocked endpoint data was great early in this project as it allowed the front end team to accelerate the build of the UI without waiting for backend infra to be put in place. Lately though, it has lead to some confusion as the endpoints have fallen out of sync. Despite efforts to improve that, this issue is a suggestion that we should remove the MSW work cleanly so that it is not a point of confusion when onboarding with the codebase.
Issue fork experience_builder-3467674
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
jessebaker commentedComment #3
hooroomooI am currently using the mocked data since it has nested components to implement the layers menu in #3462413: Implement the updated insert menu and layers panel. But once #3446722: Introduce an example set of representative SDC components; transition from "component list" to "component tree" is in and there is a way to have nested components on /node/1 then I don't care if MSW gets removed.
But as a side note, there are currently Cypress unit tests that use layout-default.json as a fixture (not MSW) so we need to make sure thats in sync. Wondering if there's a way that can check a hard-coded json file is still in sync data structure/schema-wise with what the Drupal endpoints for /api/layout returns. Or on a unit test run, if it's possible to generate a fixture based on the current data structure/schema of what /api/layout returns but maybe thats overkill for unit tests.
Comment #5
wim leersVery good point! Neither Jesse nor Ben mentioned this, so I don't think they remembered/realized that the unit tests were using the fixtures too 😅 (Or perhaps it was an implementation detail to them, not sure.)
And yes, that is possible, that is literally what we added in #3450308: Create an Open API spec for the current mock HTTP responses 👍 See
\Drupal\Tests\experience_builder\Unit\UiFixturesValidationTest.Comment #6
wim leers#3446722: Introduce an example set of representative SDC components; transition from "component list" to "component tree" is in, so @hooroomoo should not need this anymore.
Comment #7
wim leersLooks like #3468106: Render component tree inside the default theme, as a regular page broke the
pagesCI job, so now our builds look red, when really, all tests are passing 😅 Increasing priority.Comment #10
utkarsh_33 commentedFixed the test failures.There are still some warnings in the CI but that's not related to the changes that i made, so marking it needs review.
Thanks!
Comment #11
wim leersAlmost!
components.jsonshould be removed AFAICT, because it is unused.Comment #12
wim leersComment #13
utkarsh_33 commentedAddressed the feedbacks.Marking it NR
Comment #14
utkarsh_33 commentedWe cannot delete the components.json as PHP tests are using it, so added back the file.
Comment #15
wim leers👎 This is clearly the wrong conclusion.
Walking you through it:
components.json. Hence it should be removed. I explained this above.UiFixturesValidationTest::testUiComponentsFixture(), which is just making sure that this fixture complies withopenapi.ymlcomponents.jsonis gone, then that sole test method can be removed tooThe difference with
layout-default.jsonis that that file is actually used by a Cypress unit test. That's why we keep that file. And that's why continuing to test its conformance inUiFixturesValidationTest::testUiLayoutDefaultFixture()makes sense too.Comment #16
wim leersComment #17
wim leersA simple search for
mswin the codebase shows the incompleteness of this MR:package-lock.jsonstill containsmswpackage.jsonstill allow using the mocksREADME.mdstill contains instructions for how to use the mocksComment #18
utkarsh_33 commentedComment #19
wim leersI think this is ready, but needs final sign-off from a
/uiowner 😊Comment #21
wim leersComment #23
larowlanI realise I'm in the minority, but I still feel this is a mistake, we're going against best-practice in the FE world. Registering my thoughts for posterity. Hope I'm wrong.
Comment #24
wim leers@larowlan What is the mistake here in your opinion? Requiring a Drupal API server? Using server-side-generated forms? Something else?
Comment #25
larowlanToo heavy a reliance on e2e tests makes the test suite slow and brittle
Comment #26
wim leersThanks! Interesting. Didn't expect that response 😄 (Especially because this MR didn't remove any tests.)
Well, the good news is @traviscarden is improving all aspects of this project's use of OpenAPI (see #3471303: Add TravisCarden to `CODEOWNERS` for OpenAPI). So if anything, introducing functional JS or component tests tests that do not use the Drupal back end but fixtures instead will become easier, not harder.
In the current state of this project, I agree with @jessebaker's and @bnjmnm's assessment that it's the better choice to indeed only use the Drupal back end. When the UI and/or API get to a point of being more complete, I think it may make sense to reintroduce API response fixtures.