Second part of the fix for issue 17 #21
Conversation
Pull Request Test Coverage Report for Build 338
💛 - Coveralls |
There was a problem hiding this comment.
Looks mostly good, nice tidy up. Need a CHANGELOG line as some behaviour has changed - if an error occurs deleting an intermediate table during cleanup this will no longer fail the replication but will just log an error. I think that's the only user facing change?
src/main/java/com/hotels/bdp/circustrain/bigquery/partition/PartitionQueryFactory.java
Show resolved
Hide resolved
src/main/java/com/hotels/bdp/circustrain/bigquery/extraction/container/DeleteTableAction.java
Outdated
Show resolved
Hide resolved
src/test/java/com/hotels/bdp/circustrain/bigquery/client/BigQueryMetastoreClientTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/hotels/bdp/circustrain/bigquery/partition/HivePartitionKeyAdder.java
Show resolved
Hide resolved
src/main/java/com/hotels/bdp/circustrain/bigquery/extraction/container/DeleteTableAction.java
Show resolved
Hide resolved
| @@ -41,13 +55,21 @@ | |||
| private @Mock DataCleaner cleaner; | |||
| private @Mock Table table; | |||
| private @Mock ExtractionContainer container; | |||
| private @Mock PostExtractionAction postExtractionAction; | |||
| private @Mock DeleteTableAction deleteTableAction; | |||
There was a problem hiding this comment.
remove the dependency on concrete classes just mock the interface.
| } | ||
|
|
||
| @Test | ||
| public void runActionsWithTableDeletionError() { |
There was a problem hiding this comment.
Remove these tests, make one that deals with a mocked interface that throws an exception
There was a problem hiding this comment.
Why? If the DeleteTableAction has an internal failure, I need to test that it still runs, but if the other two classes that implement PostExtractionAction have an internal failure, then I need to check that other things won't run either.
I could remove them from here completely, but to keep that behaviour tested, I would move each test to the test class it belongs.
There was a problem hiding this comment.
that should be tested in the DeleteTableActionTest not here. The code doesn't know anything about DeleteTableAction neither does the test it should be decoupled.
This PR is to cleanup the code after the previous PR (#18)
It also addresses one of the unresolved comments from that PR: #18 (comment)
Fixes #17