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

Jooq into separate tag #13

Conversation

romchellis
Copy link
Contributor

Hi @sivaprasadreddy , @lukaseder .
I did suggested approach, may i get your review?

Resolves #12

@romchellis romchellis force-pushed the feature/move-jooq-to-separate-tag branch from 11e86df to 0229318 Compare June 3, 2023 07:58
pom.xml Outdated
@@ -62,7 +67,7 @@
<mysql-connector-j.version>8.0.32</mysql-connector-j.version>
<mariadb-java-client.version>3.1.2</mariadb-java-client.version>
<assertj-core.version>3.24.2</assertj-core.version>
<maven-plugin-testing-harness.version>3.3.0</maven-plugin-testing-harness.version>
<maven-plugin-testing-harness.version>4.0.0-alpha-1</maven-plugin-testing-harness.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for using Alpha version considering alpha releases might be buggy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, actually i will return

}
public class PluginTest {

@Rule
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it not possible to use JUnit 5 here? Once we introduce the dependency on JUnit 4 it will be hard to remove later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually AbstractMojoTestCase extends TestCase from Junit3 .
Usage of @Rule make possible to use Junit4 out of the box. I agree that v5 better than v4, but generally there is no difference ,because we have junit-vintage-engine which gets us backward compatibility.
I'd love to pick Junit5 ,but maven-plugin-testing-harness does not support it :(

Copy link
Contributor

@sivaprasadreddy sivaprasadreddy left a comment

Choose a reason for hiding this comment

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

The changes are looking good to me.

The only concern is introducing the dependency on JUnit 4 for testing.

Copy link
Contributor

@sivaprasadreddy sivaprasadreddy left a comment

Choose a reason for hiding this comment

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

LGTM

@sivaprasadreddy sivaprasadreddy merged commit d29e6b4 into testcontainers:main Jun 5, 2023
1 check passed
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.

Consider moving jOOQ configuration stuff into a jooq element
2 participants