XbConfigEntityHttpApiTest

I think I started testAssetLibrary() and probably copied the existing testJavaScriptComponent() as starting point . So it makes sense that since testJavaScriptComponent() didn't test this testAssetLibrary() would also miss it

For Pattern config entities, this is actually tested:

    // Use the individual URL in the list response body.
    $individual_body = $this->assertExpectedResponse('GET', Url::fromUri('base:/xb/api/config/pattern/testpatternpleaseignore'), [], 200, ['languages:language_interface', 'user.permissions', 'theme'], ['config:experience_builder.component.sdc.xb_test_sdc.props-no-slots', 'config:experience_builder.pattern.testpatternpleaseignore', 'http_response'], 'UNCACHEABLE (request policy)', 'MISS');
    $expected_individual_body_normalization = $expected_pattern_normalization;
    $expected_individual_body_normalization['js_footer'] = str_replace('xb\/api\/config\/pattern', 'xb\/api\/config\/pattern\/testpatternpleaseignore', $expected_pattern_normalization['js_footer']);
    $this->assertSame($expected_individual_body_normalization, $individual_body);

So it's only ✨ HTTP API for code component config entities Active and ✨ Storage for global CSS Active that missed this. #2 explains why both missed it.

Would be a good idea to refactor this test? looking at testPattern()don't all the entity types need to test the same things

1. Authorization, already refactored into assertAuthenticationAndAuthorization
2. Failed POST for Openapi validation
3. Failed POST for requests that pass Openapi validation
4. empty individual GET and empty list GET before entity creation
5. Successful POST
6. non-empty individual GET and non-empty list GET after entity creation
7. 409 error for trying to POST another entity using the entity id key
8. Failed PATCH for Openapi validation
9. Failed PATCH for requests that pass Openapi validation
10. Successful PATCH
11. DELETE
12. empty individual GET and empty list GET after entity deletion

Maybe this not the exact list but whatever the list is shouldn't be same for all entities?

Could we maybe make 1 test method like

 public function test(string $entity_type_id, array $openapi_invalid_data_cases, array $entity_invalid_data_cases, array $valid_initial_entity, array $expected_entity, array $valid_update_entity, $expected_updated_entity);

For

• Failed POST for Openapi validation
• Failed POST for requests that pass Openapi validation
• Failed PATCH for Openapi validation
• Failed PATCH for requests that pass Openapi validation

we could use array $openapi_invalid_data_cases, array $entity_invalid_data_cases for both cases

I think this would force use to have the same coverage for the all entity types.

If that is too many arguments to a test method we could also do,

 public function testOpenApiValidation(string $entity_type_id, array $invalid_entities, $valid_entity);

 public function testEntityiValidation(string $entity_type_id, array $invalid_entities, $valid_entity);

there are also big problems with our test coverage for non-empty list responses in `XbConfigEntityHttpApiTest`

1. \Drupal\Tests\experience_builder\Functional\XbConfigEntityHttpApiTest::testAssetLibrary doesn't have test coverage for a non-empty response from \Drupal\experience_builder\Controller\ApiConfigControllers::list

   Trying to add test coverage for this in ✨ Auto-save code components Active I found it is not possible because the response has an openapi validation error.

   We have

             description: All available asset libraries
             content:
               application/json:
                 schema:
                   type: array
   

   But the "array" schema I don't think works because Looking at \League\OpenAPIValidation\Schema\Keywords\Type::validate it has

   case CebeType::ARRAY:
                       if (! is_array($data) || ArrayHelper::isAssoc($data)) {
                           break;
                       }
   
                       $matchedType = $type;
                       break;
   

   So it doesn't match "array" for associative arrays.

2. \Drupal\Tests\experience_builder\Functional\XbConfigEntityHttpApiTest::testJavaScriptComponent does seem to test a non-empty list response but the openapi spec for /xb/api/config/js_component doesn't have GET specified so there is no openapi validation for the response
3. \Drupal\Tests\experience_builder\Functional\XbConfigEntityHttpApiTest::testPattern does seem to have test coverage for non-empty list responses but Looking at paths./xb/api/config/pattern.get in our openapi there is
   

   patternProperties:
                     '^\\[a-zA-Z0-9_-]$':
                       $ref: '#/components/schemas/PatternPreview'

   So because of 🐛 Openapi.yml uses patternProperties which is not supported by our dependencies Needs work this doesn't actually trigger validation
   I have confirmed this by switching the above to $ref: '#/components/schemas/LayoutSlot'
   and testPattern() still passes 😭

So I think they are all broken in a different way 😱

1. 😱 Excellent sleuthing! 👏
2. Yep, I've noticed there's plenty of missing OpenAPI validation. But looking at it more closely again: why is that not triggering \League\OpenAPIValidation\PSR7\Exception\NoPath?! 😱
3. 😭

This makes it all the more important to have proactive OpenAPI maintainers on this project, because it's supposed to save us time, not cost us time. And if the infrastructure doesn't let us do

It's feeling more and more that OpenAPI is less and less worth it?! 😭 Feels like we need to spend some dedicated time to make it actually robust.

So the questions we're asking now are obviously much broader than the original scope of the issue--almost existential in some ways. There's a lot of--though not total--overlap with 📌 Decide on an approach for writing tests for OpenAPI integration Active . I'll go ahead and leave my analysis here until we decide to reorganize the work. For findability, so it's not just buried in the body of this comment, I'll refer to OpenAPI.Tools, which looks very useful.

So in short, what I'm hearing is that our API development is still risky and painful due to poor specification, testing, and quality controls, leading to regressions and surprise obstacles. Specifically...

1. Our OpenAPI implementation is incomplete, untrustworthy, fragile, sometimes invalid, and inconsistently enforced.
2. Our test coverage is likewise incomplete, inconsistent, and ad hoc.

In order to stabilize API development, we need to figure out how to...

1. Ensure that openapi.yml is valid, correct, and complete. That...
   1. All API paths have an entry.
   2. Each entry specifies all of its supported methods, e.g., GET and POST.
   3. Each method specifies all known possible responses, including error responses, e.g., 200 and 500.
   4. The strictest available data validation rules are applied consistent with design requirements.
   5. The file is as correct, readable, and idiomatic as possible.
   6. It would also be nice to enforce some consistent formatting and reduce merge conflicts.
2. Get it to actually be consistently enforced on API messages (requests/responses).
3. Ensure our tests are coextensive with it and both are complete.
4. And ensure that it all stays that way.

Here are my thoughts on solutions, taking for granted that they will still revolve around openapi.yml and PHPUnit:

1. We should consider static analysis that goes deeper than our PHPUnit tests of specification itself currently do. It looks like there are a lot of options out there. A good linter that runs on CI seems like an obvious opportunity.
2. We should consider whether thephpleague/openapi-psr7-validator is (still) the best solution for enforcing the specification.
3. We might consider publishing an OpenAPI documentation site generated from our openapi.yml. That would mostly likely prevent certain errors from creeping in, and it would probably have value in its own right, since anybody in the community could access it.
4. We can either write some PHPUnit "meta tests" that inspect openapi.yml and look for corresponding tests, or we can try to enforce test requirements with interfaces and test class design, or we can look into PHP attribute-based solutions to see if they can provide any help.
5. We can extract some universal but commonly overlooked test cases and centralize them. For example, instead of each developer having to remember to test for error-handling for invalid POST bodies, we could have one test that just iterates over the paths in openapi.yml or uses the autoloader to get all of our endpoint controllers and just posts garbage, asserting that it gets an error response for each one.
6. It may be possible, by extending some of the open source tools available (many of them have extension systems) or by writing something ourselves, to automatically generate some amount of openapi.yml code automatically.
7. There may be some opportunities to improve the design of the API (controller) classes to enforce some consistency--to reduce boilerplate and require a baseline of common functionality through interfaces and base classes.

That's my first round of analysis. If I were asked, I would probably suggest starting with a linter/static analysis tool, since we'd be likely to get so much out of a good one "for free". Then I would look at our validation library and either fix our implementation or replace it. After that, I would do whatever seemed the most fun at the time. 😉

Hopefully that's what you were looking for, @wim leers. Now's the right time to ask, right? When I've already done it. 😬 🙈