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

feat: generate one test case for all services within package #267

Merged
merged 6 commits into from
Oct 11, 2024

Conversation

thall
Copy link
Member

@thall thall commented Oct 11, 2024

This is a continuation of this PR.

Why

Before each service within the package had its own test function,
meaning that if you add a new service within that package, you would
need to call a newly generated function, which could easily be forgotten.

How

This commit introduces one test function for the whole package
which takes a interface which embeds the other services test interfaces.
With this in place, if you add a new service you will get a compilation
error that you no longer fulfill the interface.

thall added 3 commits October 10, 2024 16:45
In a upcoming commit, a file called `aiptest.pb.go` will be generated.
Before both filtering of which services to generate AIP tests for
and the actual rendering of the code happened in one loop.

This commit separate its into two parts:
- Collect relevant services that tests can be generated for
- Generate code for the collected services
@thall thall requested a review from a team as a code owner October 11, 2024 06:33
Copy link
Member

@radhus radhus left a comment

Choose a reason for hiding this comment

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

Nice! Let me know what you think about the change that is (maybe) breaking, I think it's fine but good to be on the same side!

internal/plugin/service.go Show resolved Hide resolved
example/freight_service_test.go Show resolved Hide resolved
Preparation for upcoming commit where one interface will be created
which embeds all service resource config providers.

Without this, there is a risk of method name collision, where
two different service resource config providers have the same methods.
@thall thall force-pushed the tra-1929-2 branch 2 times, most recently from 2a0abd7 to 5ef888d Compare October 11, 2024 07:04
Copy link
Member

@radhus radhus left a comment

Choose a reason for hiding this comment

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

LGTM! ⭐ But probably good to get another approval if there is discussion ongoing 👍

Copy link
Member

@ericwenn ericwenn left a comment

Choose a reason for hiding this comment

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

Overall LGTM!
Taking the time to consider shorter names on the generated interfaces that will be "main entrypoint" for testing is a good idea. This plugin currently generates a lot of interfaces and many of them have suffix ConfigProvider so it can be hard to find the correct one.

thall added 2 commits October 11, 2024 11:16
This is a continuation of [commit](f9ff1a1).

Before each service within the package had its own test function,
meaning that if you add a new service within that package, you would
need to call a newly generated function, which could easily be forgotten.

This commit introduces one test function for the whole package
which takes a interface which embeds the other services test interfaces.
With this in place, if you add a new service you will get a compilation
error that you no longer fulfil the interface.
@thall thall merged commit 9017b48 into master Oct 11, 2024
1 check passed
@thall thall deleted the tra-1929-2 branch October 11, 2024 09:25
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.

4 participants