Carapacik / swagger_parser

Dart package that takes an OpenApi definition file and generates REST clients based on retrofit and data classes for your project.
https://pub.dev/packages/swagger_parser
MIT License
88 stars 33 forks source link

Create testing pipeline for bulk schema testing #181

Closed dickermoshe closed 4 months ago

dickermoshe commented 4 months ago

The https://github.com/OpenAPITools/openapi-generator project has a treasure trove of schemas that can be used to find any bugs that this package may have. They require that regression tests be added after bugs are found, this means that these schemas contain almost every combination that could confuse a generator.

A quick way to help validate that this package works well is to do the following for each schema:

  1. Generate a sample Package
  2. Build a schema
  3. Run the analyzer

If any of these steps fail, it means that there is a problem. This draft PR will work on creating this pipeline.

Thoughts?

StarProxima commented 4 months ago

@dickermoshe Hi, thank you for your contribution. I had plans to add more schemas from such sources to e2e tests, which scheme generates content and compares it with a sample (expected result, which is also easy to regenerate if needed).

Can you elaborate on the steps in your testing process? And how does it differ from the current implementation of E2E tests?

There are some comments: The analyzer, let's say, will not always be able to let us know if the api clients and models were generated correctly. Maybe we need some kind of pattern matching mechanism to have a more reliable and easy to maintain way of testing. We need to look into performance, as creating packages and running the generator can take a long time, especially for hundreds of patterns. We need to keep in mind that initially we are unlikely to be able to support correct generation of absolutely all tests.

I would like to discuss more on this topic to come up with a good testing method by which we can ensure the reliability of this package, so I fully support your contribution.

dickermoshe commented 4 months ago

Pipeline Overview

The steps in the pipeline closely resemble what a user goes through when using the library. It generates a new dart project, installs swagger_parser from source and the remaining dependencies from pub.dev. It then creates a swagger_parser_config.yaml for each schema, runs swagger_parser, runs build_runner and finaly the dart analyzer to check for any static errors.

Package Health

This package right now is really broken. In my personal experience it failed every time I've used it.
However there wasn't logical bugs, it just wouldn't compile. Many times swagger_parser failed on valid schemas, other times the build_runner would crash and sometimes the generated code had static errors. I've then went back to using the other OpenAPI generators which work, but have horrible devX.

Static Testing

Writing E2E tests that cover every schema is really hard. Hopefully one day they'll be written. But in the short term writing tests the this package works at all should be a first step.

CI/CD

I'm not arguing for adding to the CI pipeline at this time. It needs to be optimized. I would view this more a development tool to fix the vast amount bugs in the package now. Instead of playing whack-a-mole with Github issues, I'm providing you with 500 hammers to hit all the moles at once.

Developer Trust

Users of a client generator need to trust the developers. Very few are validating the generated code themselves. being handed generated code that doesnt even compile due to static errors really destroys any trust. So even if:

We need to keep in mind that initially we are unlikely to be able to support correct generation of absolutely all tests.

That should result in a nice error message being thrown, not broken code.

Thanks

I really want to thank you for your work on this package. It's really great.

StarProxima commented 4 months ago

@dickermoshe Thanks for the detailed reply, I understand the basic idea.

I agree with you that there are still a lot of uncovered cases and need an approach to solve the problems efficiently.

There are some problems with running the generator not directly from the test, but with cli. The current e2e implementation uses the GenProcessor directly, which allows the debugger to be used. This is a very important aspect that allows you to quickly find and fix errors that occur. Without this, tests lose their main usefulness, especially if we want to use them as a development tool.

I agree that it would also be good to check generated code for static errors and build_runner issues.

I'm not sure that writing e2e tests is hard right now, because we can just take all these schemas directly.

I think we need a system for tests that can generate code according to a pattern and check it for static and build_runner bugs, but also be able to debug and compare to a sample (expected generation) when we have the correct generation and want to avoid regressions. If we can variably customize these parameters (whether static and build_runner should be checked, whether it should compare to a sample, and so on), we can develop a system with which we can efficiently develop the package

dickermoshe commented 4 months ago

I'll migrate the code generation to use the built in GenProcessor.

dickermoshe commented 4 months ago

I've separated the generation, build runner and analyzer into 3 steps. Run just the testSwaggerParserGeneration tests for now.

I'm new to this whole testing world, so lmk if there is anything I should change.

dickermoshe commented 4 months ago

This is the python script I used to extract the schemas from https://github.com/OpenAPITools/openapi-generator btw

from pathlib import Path
dest_dir = Path("dest")
dest_dir.mkdir(exist_ok=True)
current_dir = Path.cwd()
dest_names = []
for i in current_dir.glob("**/*.yaml"):
    if "openapi: " in i.read_text(encoding="utf8"):
        dest_name = i.name
        if dest_name in dest_names:
            dest_name = f"{i.parent.name}__{i.name}"
        dest_names.append(dest_name)
        dest_file = dest_dir / dest_name
        with dest_file.open("w", encoding="utf8") as dest_file:
            dest_file.write(i.read_text(encoding="utf8"))
dickermoshe commented 4 months ago

@StarProxima

I'm not sure that writing e2e tests is hard right now, because we can just take all these schemas directly.

If it can't generate, build and pass static analysis, you can't do e2e testing. One thing at a time

dickermoshe commented 4 months ago

I've marked the tests as skip so that CI shouldn't run these just yet. You will need to use --run-skipped to run these tests.

For now, to run these tests use: dart test --run-skipped .\test\static\static_test.dart

To run a single test use:

https://github.com/dickermoshe/swagger_parser/blob/a0050de349187f450545b417c9b6dae314f57abe/swagger_parser/test/static/static_test.dart#L15-L18

dickermoshe commented 4 months ago

@Carapacik @StarProxima At this time swagger_parser doesn't crash on valid schemas. However there are a whole bunch of issue that result in bad code being generated. I've filed many bugs but haven't had the time to investigate each one manually. There are 2 that are related to retrofit, one can be worked around, the other will need retrofit to be fixed. Will you have time you work on these and how clear are the issues?