Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG][Rust-Axum] Sample generation overwrites handwritten tests #20416

Closed
5 of 6 tasks
Victoria-Casasampere-BeeTheData opened this issue Jan 8, 2025 · 6 comments · Fixed by #20427
Closed
5 of 6 tasks

Comments

@Victoria-Casasampere-BeeTheData
Copy link
Contributor

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

Generating samples for rust-axum using ./bin/generate-samples.sh bin/configs/manual/rust-axum-* leads to the manually written tests getting overridden with the default implementation.

Theoretically, this line of code should prevent this for occurring, but the tests get reverted to the default regardless.

Tests in rust need to be present on the generation template due to how cargo test checks for tests on the library, as they cannot be made independent outside the src folder unlike other forms of testing such as cargo bench.

This behaviour is problematic as it makes updating the rust-axum samples in future updates more annoying that it could be, as the tests need to be manually reverted after being overriden by the generator, not to mention the issues that could occurr if someone forgets to do so.

A curious behaviour that happens is that while the command is running, the manually made tests get formatted before getting overwritten shortly after. This may be due cargo fmt getting called multiple times while the source is still being generated, but I thought it was worth noting in case the override is done at a later stage.

openapi-generator version

This is an issue introduced on 3d65786 for 7.11.0 and it is not a regression.

OpenAPI declaration file content or URL

Any declaration will do, but currently the issue is present on the 2 tests implemented in the commit:

Generation Details

The issue is exclusive to the rust-axum generator with the default options. See the reproduction steps listed below.

Steps to reproduce

Run:

  • ./mvnw clean
  • ./bin/generate-samples.sh bin/configs/manual/rust-axum-*
  • See how both manually written tests get restored to the default tests.rs template.
Related issues/PRs

This was pointed out in a review on #20336 and a comment was left on the source code to acknowledge the issue.

Suggest a fix

I was not able to find a way to prevent this behaviour other than calling doNotOverwrite(), which is already being done and it makes no difference.

@wing328
Copy link
Member

wing328 commented Jan 9, 2025

for other generators, we're using apiTestTemplate and modelTestTemplate, which will not be overwritten during code generation

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/RubyClientCodegen.java#L126-L127

in RustAxum server generator, I don't see these being used: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/RustAxumServerCodegen.java#L132

shall we do the same in rust axum server generator?

cc @linxGnu

@wing328
Copy link
Member

wing328 commented Jan 9, 2025

for other samples, we usually put tests in a separate file instead of using the auto-generated test files, e.g. https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/java/okhttp-gson/src/test/java/org/openapitools/client/ClientTest.java, even though these test files (with manually-written test cases) won't be overwritten but from time to time some contributors may completely purge the output folder before regenerating the samples

(and we rely on https://github.com/OpenAPITools/openapi-generator/blob/master/bin/utils/test_file_list.yaml to make sure manually written test files such as ClientTest.java won't be purged accidentally as part any PRs)

@linxGnu
Copy link
Contributor

linxGnu commented Jan 9, 2025

Please let me clarify the issue.

Here are the concerns:

Concern 1

Generating samples for rust-axum using ./bin/generate-samples.sh bin/configs/manual/rust-axum-* leads to the manually written tests getting overridden with the default implementation.

@Victoria-Casasampere-BeeTheData Either way to overcome the situation:

  1. Manual test to be written as Integration Test
  2. Manual test is written in Template Files. If it's unit test for function X, it's recommended to write it in the same file with X.

Concern 2

Theoretically, this line of code should prevent this for occurring, but the tests get reverted to the default regardless.

@wing328 by calling function .doNotOverwrite() on Supporting Files will prevent it from being overwritten?

Concern 3

A curious behaviour that happens is that while the command is running, the manually made tests get formatted before getting overwritten shortly after. This may be due cargo fmt getting called multiple times while the source is still being generated, but I thought it was worth noting in case the override is done at a later stage.

@Victoria-Casasampere-BeeTheData you can disable cargo fmt throughout postProcess flag. For example: set this to false

@linxGnu
Copy link
Contributor

linxGnu commented Jan 9, 2025

Please let me know if I missed anything @Victoria-Casasampere-BeeTheData as we are having several concerns mixing together

@wing328
Copy link
Member

wing328 commented Jan 9, 2025

@wing328 by calling function .doNotOverwrite() on Supporting Files will prevent it from being overwritten?

should be according to the function name but I'm not 100% sure as I never use it (which is written by someone else)

@Victoria-Casasampere-BeeTheData
Copy link
Contributor Author

Re @wing328

Utilizing apiTestTemplate, modelTestTemplate, among other *TemplateFiles may be worth considering, though, I don't understand their behaviour, so I will likely not be the one implementing the support for them.

I was able to fix the issue in #20427 by moving away from generating the tests with the templates, and use separate files instead, similar to the Java unit tests. The test_file_list.yaml has been updated accordingly, to prevent the existing issue of people regenerating the samples from scratch.


Re @linxGnu

Concern 1:

Thanks for the suggestion!, I was not aware that cargo test had was able to look for tests in a tests folder. I have opened #20427 that utilizes this feature to solve the issue.

Concern 2:

It appears that the value defined by .doNotOverwrite() is never read. .isCanOverwrite() is only called in one single location, and it is only used to call .doNotOverwrite() in supporting files. The value itself has no functionality. Removing it may be the best course of action to avoid future confusion; what do you think @wing328?

Concern 3:

I am aware of this functionality, I just found it interesting and odd that the code formatted before getting overwritten/updated by the generator.

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

Successfully merging a pull request may close this issue.

3 participants