Skip to content

Replace DBSelectionExtension with annotations #1828

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

Closed
wants to merge 3 commits into from

Conversation

blafond
Copy link
Member

@blafond blafond commented Jan 10, 2024

fixes #1827

Copy link
Member

@DavideD DavideD left a comment

Choose a reason for hiding this comment

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

Thanks, I gave it a quick look and left a couple of minor changes requests. I will try to review it properly tomorrow.

I think I prefer the Jupiter convention for annotations.
I would prefer something like this:

@EnabledFor(POSTGRES)
@DisabledFor(DB2)

@blafond blafond force-pushed the issue_1827 branch 2 times, most recently from b0edece to e1eba5a Compare January 16, 2024 20:24
@blafond blafond self-assigned this Jan 16, 2024
@blafond
Copy link
Member Author

blafond commented Jan 16, 2024

@DavideD I re-worked the logic to implement ExecutionCondition like you suggested. It's lighter and cleaner and seems to work as expected.

The critical classes are: DisableForDBTypeCondition and EnableForDBTypeCondition

Copy link
Member

@DavideD DavideD left a comment

Choose a reason for hiding this comment

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

Thanks @blafond, it looks much nicer now.

There are some minor changes to apply, but looks good overall.

The test related to the annotation at a method level could be grouped in a single class:

public class EnableForMethodTest  {

	@Test
	@EnableFor(value = POSTGRESQL)
	public void testEnable() {
		// Throw exception if this test is database is NOT POSTGRESQL
		assertEquals( POSTGRESQL, DatabaseConfiguration.dbType() );
	}

	@Test
	@EnableFor(value = POSTGRESQL)
	@EnableFor(value = MYSQL)
	public void testMultipleEnable() {
		// Throw exception if this test is run with POSTGRESQL or MYSQL database
		assertTrue( DatabaseConfiguration.dbType() == POSTGRESQL ||
							DatabaseConfiguration.dbType() == MYSQL );
	}

	@Test
	@EnableForGroup({
			@EnableFor(value = MYSQL),
			@EnableFor(value = POSTGRESQL)
	})
	public void testGroupEnable() {
		// Throw exception if this test is run with POSTGRESQL or MYSQL database
		assertTrue( DatabaseConfiguration.dbType() == POSTGRESQL ||
							DatabaseConfiguration.dbType() == MYSQL );
	}

       // Add the tests with disabled
}

This way we have less test classes around.
We probably could group togethere the others as well, but it would require to use subclasses and I'm not sure it's worth the effort.

@DavideD
Copy link
Member

DavideD commented Jan 17, 2024

@blafond, could you also add the issue number in the title of the commits, please? It's quite helpful.

Use this format, please:

[#1827] Commit message 

Thanks

@blafond
Copy link
Member Author

blafond commented Jan 18, 2024

Implemented your changes/suggestions and checked formatting as well in my changed files. Found a few more unnecessary lines/comments.

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.

Replace DBSelectionExtension with annotations
2 participants