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

Add a simple plugin test #87

Merged
merged 11 commits into from
Feb 4, 2020
Merged

Conversation

martinbonnin
Copy link
Contributor

Keeping the ball rolling after FOSDEM :)

This pull request will help implementing and testing gradle lazy configuration (#63)

It adds a very basic plugin test. The test just checks that the generateSwagger task is properly executed. More tests can be added later.

plugin/build.gradle.kts Outdated Show resolved Hide resolved
plugin/build.gradle.kts Outdated Show resolved Hide resolved
Copy link
Collaborator

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Hey @martinbonnin 👋 Happy to see you here!
That's a great addition to the project and we would love to see it shipped 🚢

I left some comments around, please let me know what do you think and how do you want to proceed. Thank you again for the contrib.

Comment on lines 111 to 112
tasks.withType<Test> {
dependsOn("publishDefaultPublicationToPluginTestRepository")
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can safely replace with:

tasks.withType<Test> {
    dependsOn("publishToMavenLocal")
}

And use MavenLocal for the whole testing scenario.

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 don't like that too much since mavenLocal is global to the machine and might contain other artifacts that will be pulled in tests inadvertently. Doing everything inside the repo makes sure everything is contained.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing everything inside the repo makes sure everything is contained.

I see your point, but then having this in the test gradle file would probably suffice as well:

    mavenLocal {
      content {
        includeGroup("com.yelp.codegen")
      }
    }

This will make sure mavenLocal is used only for com.yelp.codegen and nothing else.

Copy link
Contributor Author

@martinbonnin martinbonnin Feb 4, 2020

Choose a reason for hiding this comment

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

But then there's the other side of the thing. Using mavenLocal will add the plugin under test to the global repo, which a user might end up pulling a few weeks later inadvertently when wanting to pull something else from mavenLocal.

Overall, I find adding

configure<PublishingExtension> {
    repositories {
        maven {
            name = "pluginTest"
            url = uri("file://${rootProject.buildDir}/localMaven")
        }
    }
}

is a ok price to pay for proper isolation.

plugin/build.gradle.kts Outdated Show resolved Hide resolved
plugin/build.gradle.kts Show resolved Hide resolved
plugin/src/test/testProject/build.gradle.kts Show resolved Hide resolved
plugin/src/test/testProject/build.gradle.kts Outdated Show resolved Hide resolved
Copy link
Collaborator

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's fix those small issues and we're good to go 🚢
Side note: please use the "re-request review" button when you want me to do another pass on it otherwise I might miss the PR ;)

plugin/src/test/testProject/build.gradle.kts Outdated Show resolved Hide resolved
plugin/src/test/java/com/yelp/plugin/PluginTests.kt Outdated Show resolved Hide resolved
plugin/build.gradle.kts Show resolved Hide resolved
@cortinico
Copy link
Collaborator

Awesome first contrib 💪

@cortinico cortinico merged commit 80e2315 into Yelp:master Feb 4, 2020
@cortinico cortinico added this to the 1.4.0 milestone Feb 17, 2020
@cortinico cortinico added the infra PR or Issue related to project infrastructure label Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra PR or Issue related to project infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants