- Issue created by @larowlan
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
FYI: the
FieldForComponentSuggester
service added in https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... should provide a solid foundation one of the API responses we'll need () - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Any takers? Capturing the current mock requests + responses in this way sure would be beneficial, because we'd be able to evolve the OpenAPI schema, which would auto-document the evolution of the communication between client & server!
- πΊπΈUnited States pfrilling Minster, OH
Looking through the handlers mock, I started the attached OpenAPI spec. It's still a work in progress, but I think it is a good starting point. Let me know what I missed and I can continue updating the spec.
- Status changed to Needs work
5 months ago 5:01pm 13 June 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Thanks, @pfrilling! π€©
Could you convert that to an MR? π And could you add the
SchemaValidationTest
that verifies this Open API spec is itself valid? You can pretty much copy/paste it from @larowlan's work at https://git.drupalcode.org/project/decoupled_lb_api/-/commit/84bf1e54153... πHow do we validate this, and how do we keep it in sync with the reality? π€
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
We can also write a test that loads the fixtures from UI and validates them AND uses drupalGet to load the controllers and validate them.
Compliance all round πͺ
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
That'd be amazing and what I'd hope for β because in a heterogeneous project like this (geographically, timezone-wise, employment-wise, availability-wise), that's really valuable!
Hi @pfrilling,
Can you brief me about the above merge request and how this issue needs further work?- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@syeda-farheen:
- The new test is failing:
There was 1 failure: 1) Drupal\Tests\experience_builder\Unit\SchemaValidationTest::testSchemaIsValid Failed asserting that an array is empty.
- quoting @larowlan from #7:
We can also write a test that loads the fixtures from UI and validates them AND uses drupalGet to load the controllers and validate them.
Clarified issue summary.
- The new test is failing:
- Assigned to traviscarden
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Given @syeda-farheen is not responding, assuming they're no longer working on this. Asking @TravisCarden to push this to completion π
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Importance/impactfulness of this confirmed one more at π Mock fixtures missing `field_data` Needs work .
Bumping to .
- πΊπΈUnited States pfrilling Minster, OH
This has been resolved and the API Spec test should be passing now.
> # 1 The new test is failing:Do you have any examples for testing the fixtures against the spec? If not, I'll start researching how to make that happen.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
I don't β I have zero experience with Open API π
But a quick search in @larowlan's
decoupled_lb_api
module reveals https://git.drupalcode.org/project/decoupled_lb_api/-/blob/1.x/tests/src..., whoseassertDataCompliesWithApiSpecification()
achieves exactly that :) - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
The tests that we add here should fail, because they should automatically identify π Mock fixtures missing `field_data` Needs work .
- πΊπΈUnited States pfrilling Minster, OH
Tests have been added to confirm that the UI fixtures are validating against the Open API Spec. This should resolve #3.
3. Test that the fixtures in ui/src/mocks/fixtures match the OpenAPI spec, which proves the client (XB React UI) conforms to the OpenAPI spec
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
OMG that's exciting! π€© And it's passing tests, too! π
- Assigned to wim leers
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
π Implement add button for top level item (section) Fixed conflicted with this, rebased.
- Issue was unassigned.
- Status changed to Needs review
4 months ago 4:07pm 6 August 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
This was causing increasing frustration for @jessebaker, @bnjmnm and @hooroomoo, and was slowing down @tedbow while working on π Introduce an example set of representative SDC components; transition from "component list" to "component tree" Fixed , so I took this on.
This is now ready for final review.
- Assigned to bnjmnm
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@jessebaker & @bnjmnm FYI, based on the discussion yβall had yesterday, I prioritized this issue today. Result:
- 100% of XB API responses now are validated automatically
- Any occurrence of a lack of compliance with the spec now fails:
- the Cypress E2E CI job (if the server does not comply)
- πyou can see that the error response is not quite as helpful as it could be in Cypress, will create a follow-up for that π
- the phpunit CI job (if the mocks do not comply)
- πthat happened after updating the OpenAPI spec to match the server, which is the Source of Truth currently
- β this proves that the OpenAPI spec now is used to keep everything in sync
- Fixing it causes the build to pass
Result:
npm run dev
works again:(but since making these screenshots, π Implement add button for top level item (section) Fixed landed, which changed the mock fixtures, so it'll look different now)
Assigning to @bnjmnm for review, since it's close to @jessebaker's workday.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
But it still might make sense to delete the mocks+MSW functionality, if @bnjmnm and @jessebaker decide it's not relevant anymore.
However, if we do that, we should also get rid of the GitLab Pages CI job, which would remove the static UI demo.
If that's the case, then we should IMHO still land this first and then delete the mocks in a different issue. Because that'll make it easier to someday restore it if that'll ever make sense. I propose to repurpose π Mock fixtures missing `field_data` Needs work in that case.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
It's a pity we can't have MSW wire up to cy.intercept and have the best of both worlds
- Assigned to jessebaker
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@bnjmnm didn't get to this yesterday; so re-assigning to @jessebaker at his start of day π€
- Assigned to wim leers
- Status changed to RTBC
4 months ago 9:51am 7 August 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@jessebaker approved this π₯³ Hence reflecting that here by marking .
Note that the
phpunit (next major)
CI job is failing though:There was 1 error: 1) Drupal\Tests\experience_builder\Unit\OpenApiSpecValidationTest::testSpecIsValid Exception: Could not OpenAPI 3.0 schema at /builds/project/experience_builder/vendor/cebe/php-openapi/schemas/openapi-v3.0.json or /builds/project/experience_builder/vendor/cebe/php-openapi/schemas/openapi-v3.0.json. /builds/project/experience_builder/tests/src/Unit/OpenApiSpecValidationTest.php:52 /builds/project/experience_builder/vendor/bin/phpunit:122 -- 9 tests triggered 45 PHPUnit deprecations:
For those 45 deprecations, I already opened π Fix PHPUnit 11 deprecations when tested with Drupal 11 Active (it's happening on HEAD too). That 1 error I'll need to figure out here prior to merging.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Also, @jessebaker did point out that
7. If you want to run *all* tests locally, including the OpenAPI spec one: `composer require league/openapi-psr7-validator webflo/drupal-finder cebe/php-openapi --dev`
is not quite accurate. It's currently required. I did that because I want developers of XB to be aware of violations of the OpenAPI spec. But β¦ actually doing that conditionally (if those deps are installed) would be better.
Did that in https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... π
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
RE the error described in #26:
- on 10.x,
cebe/php-openapi
is getting installed: https://git.drupalcode.org/project/experience_builder/-/jobs/2359889 - β¦ but not on 11.x
WTAF?!
Well β¦ AFAICT the reason is Drupal 11 requires Symfony 7, and that is not yet allowed by
cebe/php-openapi
: https://github.com/cebe/php-openapi/pull/203 + https://github.com/cebe/php-openapi/pull/202.That last link points to https://github.com/DEVizzent/cebe-php-openapi, which β¦ is a fork because the original is no longer maintained π€·ββοΈ
- original: https://packagist.org/packages/cebe/php-openapi, 10.3M installs
- fork: https://packagist.org/packages/devizzent/cebe-php-openapi, 1.97M installs
So obviously the ecosystem is switching over. Did the same here.
Bonus: upgrade to OpenAPI 3.1 (released in Feb 2021), which was a major motivator for people forking that library! π
- on 10.x,
- Issue was unassigned.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
So I just read https://lornajane.net/posts/2020/whats-new-in-openapi-3-1 and found several things that I could not get to work to be fixed now! So refined the OpenAPI spec some more.
I noticed that the examples visible in GitLab's OpenAPI visualization at https://git.drupalcode.org/issue/experience_builder-3450308/-/blob/34503... were off β¦ so I tried improving that using https://editor.swagger.io/, but that does not support OpenAPI 3.0.
Then found https://editor-next.swagger.io/ and β¦ that works perfectly. With live updating, even!
I did not make material changes since @jessebaker approved this, I only made it not get in the way (see #27) and pass tests against Drupal 11/Symfony 7 (see #26 and #28). And then I added finishing touches (see this comment.)
So: merging π’
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Actually, the title has been inaccurate all along π Fixed that, and created a follow-up for doing that other part: π Also validate request bodies against the OpenAPI spec Downport .
-
Wim Leers β
committed f073ffb0 on 0.x authored by
pfrilling β
Issue #3450308 by Wim Leers, pfrilling, larowlan, jessebaker: Create an...
-
Wim Leers β
committed f073ffb0 on 0.x authored by
pfrilling β
- Status changed to Fixed
4 months ago 11:11am 7 August 2024 Automatically closed - issue fixed for 2 weeks with no activity.