You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2021/06/03 09:59:26 UTC
[GitHub] [shardingsphere] huangdx0726 opened a new pull request #10638: Add test for LocalTransactionManager
huangdx0726 opened a new pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638
Fixes #10493
Changes proposed in this pull request:
-
Add test for LocalTransactionManager
-
-
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-855277856
Hi @huangdx0726
The checkstyle failed, please fix it. Before commit, you may execute `mvn install` on your local.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] menghaoranss commented on a change in pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
menghaoranss commented on a change in pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#discussion_r645254075
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
Review comment:
Need summary javadoc only, please refer to other classes
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock()
+ private LocalTransactionManager localTransactionManager;
+
+ @Test
+ public void assertBegin() {
+ localTransactionManager.begin();
+ verify(localTransactionManager).begin();
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertCommit(){
+ localTransactionManager.commit();
+ verify(localTransactionManager).commit();
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertRollback(){
+ localTransactionManager.rollback();
+ verify(localTransactionManager).rollback();
+ }
+
Review comment:
remove extra blank line
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
Review comment:
add `final`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] huangdx0726 commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
huangdx0726 commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-854390090
LocalTransaction, DistributedTransaction, WithoutTransaction Should be coverd ,like BackendTransactionManagerTest .Is right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on a change in pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on a change in pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#discussion_r645253924
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
Review comment:
Please remove the Java doc.
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock()
Review comment:
Remove the redundant bracket.
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
Review comment:
Add final modifier.
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
+ @Mock
Review comment:
A blank line is required after class definition.
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock()
+ private LocalTransactionManager localTransactionManager;
+
+ @Test
+ public void assertBegin() {
+ localTransactionManager.begin();
+ verify(localTransactionManager).begin();
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertCommit(){
+ localTransactionManager.commit();
+ verify(localTransactionManager).commit();
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertRollback(){
+ localTransactionManager.rollback();
+ verify(localTransactionManager).rollback();
+ }
+
Review comment:
Remove the redundant blank line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-854413146
> LocalTransaction, DistributedTransaction, WithoutTransaction Should be coverd ,like BackendTransactionManagerTest .Is right?
The issue is adding test for LocalTransactionManager only. And here is the guide for coding: https://shardingsphere.apache.org/community/en/contribute/code-conduct/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on a change in pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on a change in pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#discussion_r645358099
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,92 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.infra.transaction.TransactionHolder;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public final class LocalTransactionManagerTest {
+
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock
+ private LocalTransactionManager localTransactionManager;
+
+ @Mock
+ private TransactionStatus transactionStatus;
+
+ @Before
+ public void setUp() {
+ when(backendConnection.getTransactionStatus()).thenReturn(transactionStatus);
+ }
+
+ @Test
+ public void assertBegin() {
+ if (!backendConnection.getTransactionStatus().isInTransaction()) {
+ backendConnection.getTransactionStatus().setInTransaction(true);
+ TransactionHolder.setInTransaction();
+ backendConnection.closeConnections(false);
+ }
+ localTransactionManager.begin();
+ verify(localTransactionManager).begin();
+ verify(transactionStatus).setInTransaction(true);
+ verify(backendConnection).closeConnections(false);
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertCommit() {
+ if (backendConnection.getTransactionStatus().isInTransaction()) {
+ try {
+ localTransactionManager.commit();
+ verify(localTransactionManager).commit();
+ } finally {
+ backendConnection.getTransactionStatus().setInTransaction(false);
+ TransactionHolder.clear();
+ }
+ }
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertRollback() {
+ if (backendConnection.getTransactionStatus().isInTransaction()) {
+ try {
+ localTransactionManager.rollback();
+ verify(localTransactionManager).rollback();
+ } finally {
+ backendConnection.getTransactionStatus().setInTransaction(false);
+ TransactionHolder.clear();
+ }
+ }
+ }
Review comment:
Hi @huangdx0726
In a test case, you may need to setup mocked variables, invoke the methods to be tested, assert the actual results and verify the invoking times of mocked objects' methods, instead of copying the original logic to the test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] huangdx0726 commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
huangdx0726 commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-854329827
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on a change in pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on a change in pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#discussion_r645253924
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
Review comment:
Please remove the Java doc.
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock()
Review comment:
Remove the redundant bracket.
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
Review comment:
Add final modifier.
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
+ @Mock
Review comment:
A blank line is required after class definition.
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock()
+ private LocalTransactionManager localTransactionManager;
+
+ @Test
+ public void assertBegin() {
+ localTransactionManager.begin();
+ verify(localTransactionManager).begin();
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertCommit(){
+ localTransactionManager.commit();
+ verify(localTransactionManager).commit();
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertRollback(){
+ localTransactionManager.rollback();
+ verify(localTransactionManager).rollback();
+ }
+
Review comment:
Remove the redundant blank line.
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,92 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.infra.transaction.TransactionHolder;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public final class LocalTransactionManagerTest {
+
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock
+ private LocalTransactionManager localTransactionManager;
+
+ @Mock
+ private TransactionStatus transactionStatus;
+
+ @Before
+ public void setUp() {
+ when(backendConnection.getTransactionStatus()).thenReturn(transactionStatus);
+ }
+
+ @Test
+ public void assertBegin() {
+ if (!backendConnection.getTransactionStatus().isInTransaction()) {
+ backendConnection.getTransactionStatus().setInTransaction(true);
+ TransactionHolder.setInTransaction();
+ backendConnection.closeConnections(false);
+ }
+ localTransactionManager.begin();
+ verify(localTransactionManager).begin();
+ verify(transactionStatus).setInTransaction(true);
+ verify(backendConnection).closeConnections(false);
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertCommit() {
+ if (backendConnection.getTransactionStatus().isInTransaction()) {
+ try {
+ localTransactionManager.commit();
+ verify(localTransactionManager).commit();
+ } finally {
+ backendConnection.getTransactionStatus().setInTransaction(false);
+ TransactionHolder.clear();
+ }
+ }
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertRollback() {
+ if (backendConnection.getTransactionStatus().isInTransaction()) {
+ try {
+ localTransactionManager.rollback();
+ verify(localTransactionManager).rollback();
+ } finally {
+ backendConnection.getTransactionStatus().setInTransaction(false);
+ TransactionHolder.clear();
+ }
+ }
+ }
Review comment:
Hi @huangdx0726
In a test case, you may need to setup mock variable, invoke the method to be tested, assert the actual results and verify the invoking times of mocked objects' methods, instead of copying the original logic to the test cases.
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,92 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.infra.transaction.TransactionHolder;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public final class LocalTransactionManagerTest {
+
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock
+ private LocalTransactionManager localTransactionManager;
+
+ @Mock
+ private TransactionStatus transactionStatus;
+
+ @Before
+ public void setUp() {
+ when(backendConnection.getTransactionStatus()).thenReturn(transactionStatus);
+ }
+
+ @Test
+ public void assertBegin() {
+ if (!backendConnection.getTransactionStatus().isInTransaction()) {
+ backendConnection.getTransactionStatus().setInTransaction(true);
+ TransactionHolder.setInTransaction();
+ backendConnection.closeConnections(false);
+ }
+ localTransactionManager.begin();
+ verify(localTransactionManager).begin();
+ verify(transactionStatus).setInTransaction(true);
+ verify(backendConnection).closeConnections(false);
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertCommit() {
+ if (backendConnection.getTransactionStatus().isInTransaction()) {
+ try {
+ localTransactionManager.commit();
+ verify(localTransactionManager).commit();
+ } finally {
+ backendConnection.getTransactionStatus().setInTransaction(false);
+ TransactionHolder.clear();
+ }
+ }
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertRollback() {
+ if (backendConnection.getTransactionStatus().isInTransaction()) {
+ try {
+ localTransactionManager.rollback();
+ verify(localTransactionManager).rollback();
+ } finally {
+ backendConnection.getTransactionStatus().setInTransaction(false);
+ TransactionHolder.clear();
+ }
+ }
+ }
Review comment:
Hi @huangdx0726
In a test case, you may need to setup mocked variables, invoke the methods to be tested, assert the actual results and verify the invoking times of mocked objects' methods, instead of copying the original logic to the test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] menghaoranss commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
menghaoranss commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-854319488
@huangdx0726 ci build failed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] menghaoranss commented on a change in pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
menghaoranss commented on a change in pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#discussion_r645254075
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
Review comment:
Need summary javadoc only, please refer to other classes
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock()
+ private LocalTransactionManager localTransactionManager;
+
+ @Test
+ public void assertBegin() {
+ localTransactionManager.begin();
+ verify(localTransactionManager).begin();
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertCommit(){
+ localTransactionManager.commit();
+ verify(localTransactionManager).commit();
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertRollback(){
+ localTransactionManager.rollback();
+ verify(localTransactionManager).rollback();
+ }
+
Review comment:
remove extra blank line
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,65 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+
+/**
+ * @Author hdx
+ * @Date 2021-06-03
+ * @Version 1.0
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class LocalTransactionManagerTest {
Review comment:
add `final`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] huangdx0726 commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
huangdx0726 commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-854329827
I have fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] huangdx0726 closed pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
huangdx0726 closed pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] codecov-commenter commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-854452511
# [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/10638?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10638](https://codecov.io/gh/apache/shardingsphere/pull/10638?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (198fae5) into [master](https://codecov.io/gh/apache/shardingsphere/commit/b47b6e256bffbd6a7fab2d6afa743ec38e36270d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b47b6e2) will **decrease** coverage by `0.61%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/shardingsphere/pull/10638?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #10638 +/- ##
============================================
- Coverage 68.92% 68.31% -0.62%
- Complexity 693 727 +34
============================================
Files 1728 1773 +45
Lines 29490 30537 +1047
Branches 5299 5488 +189
============================================
+ Hits 20327 20861 +534
- Misses 7592 8047 +455
- Partials 1571 1629 +58
```
| [Impacted Files](https://codecov.io/gh/apache/shardingsphere/pull/10638?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../transaction/TransactionBackendHandlerFactory.java](https://codecov.io/gh/apache/shardingsphere/pull/10638/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtcHJveHkvc2hhcmRpbmdzcGhlcmUtcHJveHktYmFja2VuZC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcHJveHkvYmFja2VuZC90ZXh0L3RyYW5zYWN0aW9uL1RyYW5zYWN0aW9uQmFja2VuZEhhbmRsZXJGYWN0b3J5LmphdmE=) | `64.70% <0.00%> (-35.30%)` | :arrow_down: |
| [...merge/result/impl/memory/MemoryQueryResultRow.java](https://codecov.io/gh/apache/shardingsphere/pull/10638/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtaW5mcmEvc2hhcmRpbmdzcGhlcmUtaW5mcmEtbWVyZ2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL2luZnJhL21lcmdlL3Jlc3VsdC9pbXBsL21lbW9yeS9NZW1vcnlRdWVyeVJlc3VsdFJvdy5qYXZh) | `65.00% <0.00%> (-35.00%)` | :arrow_down: |
| [...dialect/handler/ddl/DropIndexStatementHandler.java](https://codecov.io/gh/apache/shardingsphere/pull/10638/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLXN0YXRlbWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvc3FsL3BhcnNlci9zcWwvZGlhbGVjdC9oYW5kbGVyL2RkbC9Ecm9wSW5kZXhTdGF0ZW1lbnRIYW5kbGVyLmphdmE=) | `30.00% <0.00%> (-30.00%)` | :arrow_down: |
| [...sphere/scaling/core/job/schedule/JobScheduler.java](https://codecov.io/gh/apache/shardingsphere/pull/10638/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtc2NhbGluZy9zaGFyZGluZ3NwaGVyZS1zY2FsaW5nLWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL3NjYWxpbmcvY29yZS9qb2Ivc2NoZWR1bGUvSm9iU2NoZWR1bGVyLmphdmE=) | `22.22% <0.00%> (-29.63%)` | :arrow_down: |
| [...atement/impl/PostgreSQLTCLStatementSQLVisitor.java](https://codecov.io/gh/apache/shardingsphere/pull/10638/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLWRpYWxlY3Qvc2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci1wb3N0Z3Jlc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9zcWwvcGFyc2VyL3Bvc3RncmVzcWwvdmlzaXRvci9zdGF0ZW1lbnQvaW1wbC9Qb3N0Z3JlU1FMVENMU3RhdGVtZW50U1FMVmlzaXRvci5qYXZh) | `71.42% <0.00%> (-28.58%)` | :arrow_down: |
| [...atement/impl/PostgreSQLDALStatementSQLVisitor.java](https://codecov.io/gh/apache/shardingsphere/pull/10638/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLWRpYWxlY3Qvc2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci1wb3N0Z3Jlc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9zcWwvcGFyc2VyL3Bvc3RncmVzcWwvdmlzaXRvci9zdGF0ZW1lbnQvaW1wbC9Qb3N0Z3JlU1FMREFMU3RhdGVtZW50U1FMVmlzaXRvci5qYXZh) | `64.91% <0.00%> (-28.20%)` | :arrow_down: |
| [...rser/sql/common/statement/dml/SelectStatement.java](https://codecov.io/gh/apache/shardingsphere/pull/10638/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLXN0YXRlbWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvc3FsL3BhcnNlci9zcWwvY29tbW9uL3N0YXRlbWVudC9kbWwvU2VsZWN0U3RhdGVtZW50LmphdmE=) | `80.00% <0.00%> (-20.00%)` | :arrow_down: |
| [...a/merge/result/impl/memory/MemoryMergedResult.java](https://codecov.io/gh/apache/shardingsphere/pull/10638/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtaW5mcmEvc2hhcmRpbmdzcGhlcmUtaW5mcmEtbWVyZ2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL2luZnJhL21lcmdlL3Jlc3VsdC9pbXBsL21lbW9yeS9NZW1vcnlNZXJnZWRSZXN1bHQuamF2YQ==) | `65.38% <0.00%> (-19.62%)` | :arrow_down: |
| [...nd/text/transaction/TransactionBackendHandler.java](https://codecov.io/gh/apache/shardingsphere/pull/10638/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtcHJveHkvc2hhcmRpbmdzcGhlcmUtcHJveHktYmFja2VuZC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcHJveHkvYmFja2VuZC90ZXh0L3RyYW5zYWN0aW9uL1RyYW5zYWN0aW9uQmFja2VuZEhhbmRsZXIuamF2YQ==) | `45.00% <0.00%> (-19.29%)` | :arrow_down: |
| [...dl/impl/CreateShardingTableRuleBackendHandler.java](https://codecov.io/gh/apache/shardingsphere/pull/10638/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtcHJveHkvc2hhcmRpbmdzcGhlcmUtcHJveHktYmFja2VuZC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcHJveHkvYmFja2VuZC90ZXh0L2Rpc3RzcWwvcmRsL2ltcGwvQ3JlYXRlU2hhcmRpbmdUYWJsZVJ1bGVCYWNrZW5kSGFuZGxlci5qYXZh) | `80.00% <0.00%> (-11.67%)` | :arrow_down: |
| ... and [303 more](https://codecov.io/gh/apache/shardingsphere/pull/10638/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/shardingsphere/pull/10638?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/10638?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b47b6e2...198fae5](https://codecov.io/gh/apache/shardingsphere/pull/10638?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-855586166
Hi @huangdx0726
Your branch is too far behind the branch `apache:master`. Please rebase your branch onto the `apache:master`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] huangdx0726 commented on a change in pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
huangdx0726 commented on a change in pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#discussion_r645415552
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,92 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.infra.transaction.TransactionHolder;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public final class LocalTransactionManagerTest {
+
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock
+ private LocalTransactionManager localTransactionManager;
+
+ @Mock
+ private TransactionStatus transactionStatus;
+
+ @Before
+ public void setUp() {
+ when(backendConnection.getTransactionStatus()).thenReturn(transactionStatus);
+ }
+
+ @Test
+ public void assertBegin() {
+ if (!backendConnection.getTransactionStatus().isInTransaction()) {
+ backendConnection.getTransactionStatus().setInTransaction(true);
+ TransactionHolder.setInTransaction();
+ backendConnection.closeConnections(false);
+ }
+ localTransactionManager.begin();
+ verify(localTransactionManager).begin();
+ verify(transactionStatus).setInTransaction(true);
+ verify(backendConnection).closeConnections(false);
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertCommit() {
+ if (backendConnection.getTransactionStatus().isInTransaction()) {
+ try {
+ localTransactionManager.commit();
+ verify(localTransactionManager).commit();
+ } finally {
+ backendConnection.getTransactionStatus().setInTransaction(false);
+ TransactionHolder.clear();
+ }
+ }
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertRollback() {
+ if (backendConnection.getTransactionStatus().isInTransaction()) {
+ try {
+ localTransactionManager.rollback();
+ verify(localTransactionManager).rollback();
+ } finally {
+ backendConnection.getTransactionStatus().setInTransaction(false);
+ TransactionHolder.clear();
+ }
+ }
+ }
Review comment:
Thx, I'll try again. Please take a look later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] menghaoranss commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
menghaoranss commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-854319488
@huangdx0726 ci build failed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on a change in pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on a change in pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#discussion_r645358099
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,92 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.infra.transaction.TransactionHolder;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.SQLException;
+
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public final class LocalTransactionManagerTest {
+
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock
+ private LocalTransactionManager localTransactionManager;
+
+ @Mock
+ private TransactionStatus transactionStatus;
+
+ @Before
+ public void setUp() {
+ when(backendConnection.getTransactionStatus()).thenReturn(transactionStatus);
+ }
+
+ @Test
+ public void assertBegin() {
+ if (!backendConnection.getTransactionStatus().isInTransaction()) {
+ backendConnection.getTransactionStatus().setInTransaction(true);
+ TransactionHolder.setInTransaction();
+ backendConnection.closeConnections(false);
+ }
+ localTransactionManager.begin();
+ verify(localTransactionManager).begin();
+ verify(transactionStatus).setInTransaction(true);
+ verify(backendConnection).closeConnections(false);
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertCommit() {
+ if (backendConnection.getTransactionStatus().isInTransaction()) {
+ try {
+ localTransactionManager.commit();
+ verify(localTransactionManager).commit();
+ } finally {
+ backendConnection.getTransactionStatus().setInTransaction(false);
+ TransactionHolder.clear();
+ }
+ }
+ }
+
+ @Test
+ @SneakyThrows(SQLException.class)
+ public void assertRollback() {
+ if (backendConnection.getTransactionStatus().isInTransaction()) {
+ try {
+ localTransactionManager.rollback();
+ verify(localTransactionManager).rollback();
+ } finally {
+ backendConnection.getTransactionStatus().setInTransaction(false);
+ TransactionHolder.clear();
+ }
+ }
+ }
Review comment:
Hi @huangdx0726
In a test case, you may need to setup mock variable, invoke the method to be tested, assert the actual results and verify the invoking times of mocked objects' methods, instead of copying the original logic to the test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on a change in pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on a change in pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#discussion_r646021668
##########
File path: shardingsphere-proxy/shardingsphere-proxy-backend/src/test/java/org/apache/shardingsphere/proxy/backend/communication/jdbc/transaction/LocalTransactionManagerTest.java
##########
@@ -0,0 +1,93 @@
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction;
+
+import com.google.common.collect.HashMultimap;
+import com.google.common.collect.Multimap;
+import lombok.SneakyThrows;
+import org.apache.shardingsphere.infra.transaction.TransactionHolder;
+import org.apache.shardingsphere.proxy.backend.communication.jdbc.connection.BackendConnection;
+import org.apache.shardingsphere.transaction.core.TransactionType;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public final class LocalTransactionManagerTest {
+
+ @Mock
+ private BackendConnection backendConnection;
+
+ @Mock
+ private TransactionStatus transactionStatus;
+
+ @Mock
+ private Connection connection;
+
+ private LocalTransactionManager localTransactionManager;
+
+ @Before
+ public void setUp() throws ReflectiveOperationException, SQLException {
Review comment:
Can the `ReflectiveOperationException` be removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-854331660
Hi @huangdx0726
Could you please have a look at `org.apache.shardingsphere.proxy.backend.communication.jdbc.transaction.BackendTransactionManagerTest`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
TeslaCN commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-854331660
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] huangdx0726 commented on pull request #10638: Add test for LocalTransactionManager
Posted by GitBox <gi...@apache.org>.
huangdx0726 commented on pull request #10638:
URL: https://github.com/apache/shardingsphere/pull/10638#issuecomment-854390207
@TeslaCN
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org