ThirdPartySyncCommand - Adding new parameters #614
Conversation
cli/src/test/java/com/box/l10n/mojito/cli/command/ThirdPartySyncCommandTest.java
Show resolved
Hide resolved
cli/src/test/java/com/box/l10n/mojito/cli/command/ThirdPartySyncCommandTest.java
Show resolved
Hide resolved
|
@aurambaj Created this new PR to merge into |
cli/src/test/java/com/box/l10n/mojito/cli/command/ThirdPartySyncCommandTest.java
Show resolved
Hide resolved
| @@ -1,15 +1,15 @@ | |||
| package com.box.l10n.mojito.cli.command; | |||
|
|
|||
| import com.box.l10n.mojito.rest.client.ThirdPartySync; | |||
| import com.box.l10n.mojito.rest.ThirdPartySyncAction; | |||
There was a problem hiding this comment.
oh sorry, maybe I wasn't super clear last time we chatted about it. I don't want the rest client and the webservice layer to be linked closely (with webapp depending on the client). The main reason is that in most cases there is not a 1:1 mapping between what's done in the server and what makes sense in the client (hibernate entities, slightly different object). I know that in many case the POJO are or feel like duplicates between the client and server but I'd rather keep that way. Let's not have the rest client as dependency of webapp, it also becomes messy and we may end up having some code going through the client instead of directly through the service which doesn't make sense. It may become confusing.
There was a problem hiding this comment.
Just updated the code
|
|
||
| String outputString = outputCapture.toString(); | ||
| assertTrue(outputString.contains(Arrays.asList(ThirdPartySync.Action.MAP_TEXTUNIT, ThirdPartySync.Action.PUSH_SCREENSHOT).toString())); |
| "-sa", skipAssetPattern, | ||
| "-o", options.get(0), options.get(1)); | ||
|
|
||
| waitForCondition("Ensure ThirdPartySyncJob gets executed", |
There was a problem hiding this comment.
I guess that listener is ok for now though I think changing the application context is not too great if we ever start running test in parallel, etc.
So my preference would be avoiding the listener and just wait for the state (or different part of the state) to get to what we expect which would indicate a successful migration.
On top of my head I don't remember other test intercepting part of the request to perform input validation. In general those are more like end to end test. The parameters are not check directly but instead the results are checked inferring that the proper arguments were passed
There was a problem hiding this comment.
I've prioritized on testing that the parameters given to the CLI were passed around downstream all the way to the Quartz job for this test. I do agree that here is better to test final states or side effects, but since non of the parameters is being used in ThirdPartyTMSInMemory method we have nowadays and we're not triggering side effects, my preference for this PR was to have the argument passing fully tested (since that's what we're changing now).
Since we'll still implement new methods in ThirdPartyTMS in the upcoming PRs, I will add end to end testing through the ThirdPartyTMSInMemory class once we have these new method added to the interface signature as well as to the interface implementors (ThirdPartyTMSSmartling and ThirdPartyTMSInMemory)
To implement the
PULL,PUSHandPUSH_TRANSLATIONScommands inThirdPartyService, we need to updateThirdPartySyncCommandand several dependencies to utilize these new parameters:--skip-text-units-with-patternand--skip-assets-path-pattern.Please note the TODO added to
ThirdPartySyncCommandTestand documented in #612