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.

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

jessebaker created an issue. See original summary.

jessebaker’s picture

Assigned: Unassigned » jessebaker
hooroomoo’s picture

I 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.

Wim Leers credited bnjmnm.

wim leers’s picture

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.

Very 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.

wim leers’s picture

Title: Remove MSW infrastucture » Remove MSW infrastucture + `pages` CI job
Assigned: jessebaker » Unassigned
Issue tags: +Novice
wim leers’s picture

Priority: Normal » Major

Looks like #3468106: Render component tree inside the default theme, as a regular page broke the pages CI job, so now our builds look red, when really, all tests are passing 😅 Increasing priority.

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

utkarsh_33’s picture

Assigned: Unassigned » wim leers
Status: Active » Needs review

Fixed 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!

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs review » Needs work

Almost! components.json should be removed AFAICT, because it is unused.

wim leers’s picture

Assigned: Unassigned » utkarsh_33
utkarsh_33’s picture

Assigned: utkarsh_33 » Unassigned
Status: Needs work » Needs review

Addressed the feedbacks.Marking it NR

utkarsh_33’s picture

We cannot delete the components.json as PHP tests are using it, so added back the file.

wim leers’s picture

Assigned: Unassigned » utkarsh_33
Status: Needs review » Needs work

We cannot delete the components.json as PHP tests are using it, so added back the file.

👎 This is clearly the wrong conclusion.

Walking you through it:

  1. nothing is using components.json. Hence it should be removed. I explained this above.
  2. the only thing that failed is UiFixturesValidationTest::testUiComponentsFixture(), which is just making sure that this fixture complies with openapi.yml
  3. … so clearly, if components.json is gone, then that sole test method can be removed too

The difference with layout-default.json is 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 in UiFixturesValidationTest::testUiLayoutDefaultFixture() makes sense too.

wim leers’s picture

Assigned: utkarsh_33 » wim leers
Status: Needs work » Needs review
wim leers’s picture

Assigned: wim leers » utkarsh_33
Status: Needs review » Needs work

A simple search for msw in the codebase shows the incompleteness of this MR:

  1. package-lock.json still contains msw
  2. the scripts in package.json still allow using the mocks
  3. README.md still contains instructions for how to use the mocks
utkarsh_33’s picture

Assigned: utkarsh_33 » wim leers
Status: Needs work » Needs review
wim leers’s picture

Assigned: wim leers » balintbrews
Status: Needs review » Reviewed & tested by the community

I think this is ready, but needs final sign-off from a /ui owner 😊

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

wim leers’s picture

Assigned: balintbrews » Unassigned
Status: Reviewed & tested by the community » Fixed

  • Wim Leers committed f5d5b244 on 0.x authored by utkarsh_33
    Issue #3467674 by utkarsh_33, balintbrews, Wim Leers, jessebaker, bnjmnm...
larowlan’s picture

I 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.

wim leers’s picture

@larowlan What is the mistake here in your opinion? Requiring a Drupal API server? Using server-side-generated forms? Something else?

larowlan’s picture

Too heavy a reliance on e2e tests makes the test suite slow and brittle

wim leers’s picture

Thanks! 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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.