Skip to content

Refactor out common test classes into new test project#2350

Merged
loosebazooka merged 5 commits intomasterfrom
common-testing
Mar 31, 2020
Merged

Refactor out common test classes into new test project#2350
loosebazooka merged 5 commits intomasterfrom
common-testing

Conversation

@loosebazooka
Copy link
Member

part of #2150

  • create new test subproject (jib-testing-common)
  • move common classes into new project (ideally this should be enough for all our cross project test infra sharing)
  • checkstyle suppression for jib-testing-common
  • NOT DONE: shared resources moved into jib-testing-common

@loosebazooka loosebazooka requested a review from a team March 20, 2020 19:33
@loosebazooka loosebazooka mentioned this pull request Mar 20, 2020
3 tasks
@loosebazooka
Copy link
Member Author

loosebazooka commented Mar 20, 2020

oh boy, gotta fix these javadoc issues

hrmm, something strange is happening with packaging now, not sure why

Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

Should we move these too?

HttpGetVerifier
PlainHttpClient
RequestWrapper

And I guess it's not our intention to move Gradle and Maven-specific test helpers, right?

  • Gradle: TestProject, JibRunHelper
  • Maven: TestProject, TestRepository, SkippedGoalVerifier, SettingsFixture

<suppress files=".*[\\/]HelpfulSuggestions\.java" checks="MissingJavadocMethod"/>

<!-- leniency for test files -->
<suppress files=".*[\\/]jib-testing-common[\\/].*\.java" checks ="MissingJavadocMethod"/>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can start the pattern with jib-testing-common[\\/] without ."[\\/]?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if it matters, I guess this is a little more strict: /jib-testing-common/ instead of abc-jib-testing-common/


testImplementation project(path:':jib-plugins-common', configuration:'tests')
integrationTestImplementation project(path:':jib-core', configuration:'integrationTests')
testImplementation project(path:':jib-testing-common')
Copy link
Member

Choose a reason for hiding this comment

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

nit: for consistency, testImplementation project(':jib-testing-common') (without specifying the path argument name).

And does the order of testImplementation matter? In jib-core, it's the first entry.


testImplementation project(path:':jib-plugins-common', configuration:'tests')
integrationTestImplementation project(path:':jib-core', configuration:'integrationTests')
testImplementation project(path:':jib-testing-common')
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@loosebazooka
Copy link
Member Author

Should we move these too?

HttpGetVerifier
PlainHttpClient
RequestWrapper

So these aren't shared by anyone, I guess if we want to eventually export them we can? But there's no need to now.

@loosebazooka loosebazooka merged commit dce2f87 into master Mar 31, 2020
@loosebazooka loosebazooka deleted the common-testing branch March 31, 2020 02:08
chanseokoh added a commit that referenced this pull request Mar 31, 2020
chanseokoh added a commit that referenced this pull request Mar 31, 2020
loosebazooka added a commit that referenced this pull request Mar 31, 2020
loosebazooka added a commit that referenced this pull request Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants