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

Delete autogenerated test stubs #149

Merged
merged 2 commits into from
Jul 24, 2023
Merged

Delete autogenerated test stubs #149

merged 2 commits into from
Jul 24, 2023

Conversation

maxb
Copy link
Contributor

@maxb maxb commented Jul 23, 2023

I propose that we delete the autogenerated test stubs.

They are only stubs, containing commented out code to serve as
a starting point for human written tests - they do not actually test
anything.

I suggest that it is unlikely that the project will ever make use of
these test stubs for the purpose intended by the generator - I do not
think anyone is going to be going through the API manually filling in
tests that exercise all/most client methods against a Vault server.

Additionally, in the case of the src/Vault.Test/Api/*Tests.cs files,
they are generated by openapi-generator only if they do not already
exist, and as a result are already out of date with various method
renames that have occurred, as OpenAPI operation IDs have been tweaked
over time.

I have retained one file of non-generated tests.

This change harmonizes with the sibling vault-client-go project, which
already has generation of test stubs turned off.

I propose that we delete the autogenerated test stubs.

They are only stubs, containing commented out code to serve as
a starting point for human written tests - they do not actually test
anything.

I suggest that it is unlikely that the project will ever make use of
these test stubs for the purpose intended by the generator - I do not
think anyone is going to be going through the API manually filling in
tests that exercise all/most client methods against a Vault server.

Additionally, in the case of the `src/Vault.Test/Api/*Tests.cs` files,
they are generated by openapi-generator only if they do not already
exist, and as a result are already out of date with various method
renames that have occurred, as OpenAPI operation IDs have been tweaked
over time.

I have retained one file of non-generated tests.

This change harmonizes with the sibling vault-client-go project, which
already has generation of test stubs turned off.
@maxb maxb requested a review from a team July 23, 2023 06:19
@maxb
Copy link
Contributor Author

maxb commented Jul 23, 2023

This will also fix the "Unchanged files with check annotations" section showing up in every PR diff, complaining about unused code in the tests.

# For example, you can ignore all files in a docs folder with the file extension .md:
#docs/*.md
# Then explicitly reverse the ignore rule for a single file:
#!docs/README.md
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simplified this comment to match the one in sibling project vault-client-go.

Copy link
Collaborator

@averche averche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. If we ever want to experiment with the generated tests, we can easily revert this :)

Thanks for the PR!

@maxb
Copy link
Contributor Author

maxb commented Jul 23, 2023

Conflict resolved.

Copy link
Contributor

@AnPucel AnPucel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always happy to unnecessary code being removed!

@maxb
Copy link
Contributor Author

maxb commented Jul 24, 2023

Thanks - please go ahead and merge, if you are happy.

@AnPucel AnPucel merged commit 442b32b into hashicorp:main Jul 24, 2023
4 checks passed
@maxb maxb deleted the delete-tests branch July 24, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants