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

#2126 add SQLLoggerTest #2127

Closed
wants to merge 2 commits into from
Closed

#2126 add SQLLoggerTest #2127

wants to merge 2 commits into from

Conversation

betterjava
Copy link
Member

@betterjava betterjava commented Mar 28, 2019

Fixes #2126 .

Changes proposed in this pull request:

  • add SQLLogTest to test SQLLogger

@betterjava betterjava changed the title add SQLLoggerTest #2126 add SQLLoggerTest Mar 28, 2019

public final class SQLLoggerTest {

private String sql;
Copy link
Member

Choose a reason for hiding this comment

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

Fields should have a blank line each other

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok,I will fix this.

import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;

public final class SQLLoggerTest {
Copy link
Member

Choose a reason for hiding this comment

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

Should use mock, not test real logback

Copy link
Member Author

Choose a reason for hiding this comment

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

Mockito can not support mock static method. Should I add powermock to the project dependency tree?

@terrymanu
Copy link
Member

log.info() is not a static method

@betterjava
Copy link
Member Author

I can not find a way to mock a logger for SQLLogger. Mockito can create a mock for Logger,but I can not figure out how to replace real logger with the mock logger.So I close the pull now.

@betterjava betterjava closed this Mar 29, 2019
@betterjava
Copy link
Member Author

betterjava commented Mar 29, 2019

@terrymanu PowerMock can do this work.

        Logger log = PowerMockito.mock(Logger.class);
        PowerMockito.mockStatic(LoggerFactory.class);
        PowerMockito.when(LoggerFactory.getLogger("ShardingSphere-SQL")).thenReturn(log);
        SQLLogger.logSQL("select * from user", Arrays.asList("db1", "db2"));
        InOrder inOrder = Mockito.inOrder(log);
        inOrder.verify(log).info("Rule Type: master-slave", new Object[]{});
        inOrder.verify(log).info("SQL: {} ::: DataSources: {}", new Object[]{ "select * from user","db1,db2"});

@terrymanu
Copy link
Member

Import new mock dependency is unnecessary.
method log.info() is not static method, why we have to import power mock to mock this?

@terrymanu
Copy link
Member

You can use reflection to inject mocked logger

@betterjava
Copy link
Member Author

Thanks,I will try.

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.

2 participants