Skip to content

Conversation

@gcatanese
Copy link
Contributor

Update CI pipeline to verify the go-server generated output is correct. Python tests have been created to check the content of the Go files

@wing328 Similarly done as for the postman-collection generator

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@wing328
Copy link
Member

wing328 commented Sep 2, 2024

instead of writing the test in python in the output folder, have you considered adding java unit tests similar to https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/java/org/openapitools/codegen/goginserver/GoGinServerCodegenTest.java ?

@gcatanese
Copy link
Contributor Author

Yes, that works but I thought that using Python (or Node) tests allows to better verify the output of the generated code:

  • tests verify the official samples
  • when writing/running tests the generated samples are available (making a lot easier to debug errors)
  • Python tests are lighter and more readable

After introducing this in the postman-collection generator I thought it would be the preferred approach for validating the output. Don't you think we could adopt this going forward?

@wing328
Copy link
Member

wing328 commented Sep 4, 2024

I don't think go contributors need to learn python/node in order to write tests for the go (client/server) generators (even though python/node is not hard to learn)

@gcatanese
Copy link
Contributor Author

I understand your concern, but for example when working on #19550 I find a lot more challenging to create the Java tests, looking at temporary files created (disabling the File::deleteOnExit) to look at the output. At each test execution I have to look into a different (randomly generated) folder to see the new output.

When verifying the samples (with Python) the generated output is in front of me, and I only have to work/check those files.
Additionally, the samples become richer, including more examples as new tests are created.

Why not allowing both approaches?

@wing328
Copy link
Member

wing328 commented Sep 13, 2024

When verifying the samples (with Python) the generated output is in front of me, and I only have to work/check those files.

My question is more on why not writing these tests in Go instead of Python (or other languages) given that the output that needs to be verified are Go code anyway?

I still think writing these tests in Go make other Go contributors life easier as they may not even have Python installed locally in their environment to run the tests.

@gcatanese
Copy link
Contributor Author

Ah ok, I see it, I think it is a good idea. I will try it out and update the PR

@gcatanese
Copy link
Contributor Author

@wing328 I have replaced the Python tests with Go tests, please have a look

@wing328 wing328 merged commit 2f179fe into OpenAPITools:master Sep 16, 2024
@wing328
Copy link
Member

wing328 commented Sep 16, 2024

Appreciate the PR to add more tests 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants