We need to decide how to write tests for our OpenAPI integration--both strategy and tactics. For instance, is the goal to verify that openapi.yml works with expected values? Or is the goal to create kernel-or-unit tests that API messages conform to the standard without relying on functional tests verifying it indirectly and incidentally? Or some combination of the two? Then, obviously, we need to set a precedent and make it reusable. Remember to consider error responses.

Issue fork experience_builder-3472632

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:

About issue forks

Comments

traviscarden created an issue. See original summary.

traviscarden’s picture

traviscarden commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

IMHO functional tests (BrowserTestBase + Cypress E2E tests) is sufficient for now — that's sufficient for testing the common paths.

The goal for OpenAPI is to help the front and back ends to stay in sync with more ease, not for that to become a goal unto itself.

IOW:

  1. OpenAPI to ensure valid request & response shapes, running only during development
  2. actual validation of requests when those requests mutate server-side state — i.e. this is what src/Plugin/Validation/Constraint/* will be for.

That being said, if you see a low-effort/maintenance, high-impact/reach way to write kernel or unit tests to verify certain edge cases in request/response bodies are caught automatically by OpenAPI, then I'd definitely welcome that :) But that should not be a multi-week/month endeavor IMHO.

traviscarden opened merge request !284

traviscarden’s picture

traviscarden commented
Assigned: traviscarden » wim leers
Status: Active » Needs review

tl;dr: I agree with everything you said, and I have an "inexpensive" solution I would like to implement that I would consider to satisfy the requirements of this issue.

IMHO functional tests (BrowserTestBase + Cypress E2E tests) is sufficient for now — that's sufficient for testing the common paths.

I agree.

The goal for OpenAPI is to help the front and back ends to stay in sync with more ease, not for that to become a goal unto itself.

Yes. We're on the same page here.

That being said, if you see a low-effort/maintenance, high-impact/reach way to write kernel or unit tests to verify certain edge cases in request/response bodies are caught automatically by OpenAPI, then I'd definitely welcome that :)

I have proposed one in the above MR. I actually hadn't planned on looking at this issue at all yet, but working on #3472584: Add missing request body schemas to `openapi.yml` I was having a hard time testing my assumptions about how the specification itself was being interpreted. And the ability to just whip up a quick PHP array and get instant feedback really clarifies things and saves a lot of "throwing things at the wall" and "testing by coincidence". (Turns out even YAML can be written test-first! 😄)

But that should not be a multi-week/month endeavor IMHO.

It just took a day. 😛

Re: the MR

The tests are passing, but I notice PHPUnit reports two tests skipped--which I assume are the two that this MR adds. That is doubtless because the tests depend on league/openapi-psr7-validator, which I assume isn't installed on CI. Ordinarily that would constitute a problem that needs solving, of course, but I'm actually fine with it in this case, since its whole point is to aid in rapid development and the real "regression tests" are in Cypress. Developers can just run them ad hoc in PhpStorm, for example (as I've been doing), when actually modifying openapi.yml and ignore them the rest of the time. They could technically get out of sync, but as far as I'm concerned, we can create a follow-up to worry about that later.

So I would like to propose the MR above as a rapid development tool that satisfies our minimal requirements and lays a foundation of future iteration if that ever seems valuable. According to your judgment, we can create a follow-up issue and/or a @todo or a note in the README I added to capture the above issue of the tests not getting run on CI.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » traviscarden
Status: Needs review » Needs work

On the right track 👍 … but it's too PHP-heavy. This test coverage must have a very low bar for non-PHP developers. See MR with suggestions.

traviscarden’s picture

traviscarden commented

FYI: I'm setting this issue aside for the time being in favor of higher priorities.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Version: » 0.x-dev
Component: Page builder » Code
Status: Needs work » Postponed (maintainer needs more info)

Following up on non-draft MRs that have been open for a long time. That led me here. What's next here? Is this still relevant?

wim leers closed merge request !284

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Component: Code » Project management
Assigned: traviscarden » Unassigned
Status: Postponed (maintainer needs more info) » Closed (works as designed)
Issue tags: +DX (Developer Experience)
Related issues: +#3499703: Make all XB HTTP API routes consistently prefixed, ensure they all have OpenAPI specs, and tests to keep it so

I think #3499703: Make all XB HTTP API routes consistently prefixed, ensure they all have OpenAPI specs, and tests to keep it so — which I added ~1 month after #8 — is sufficient.

It enforces the presence of OpenAPI specs for every route, in a consistent manner. Testing OpenAPI specs in detail is overhead not worth it to this project, because it's meant to accelerate our "internal velocity": it's not a public HTTP API intended to have an perfect OpenAPI spec; the OpenAPI spec is meant to help us develop faster and catch glaring problems early on.