-
Notifications
You must be signed in to change notification settings - Fork 37
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
Migrate product sync and ITs to java sdk v2 #947
Migrate product sync and ITs to java sdk v2 #947
Conversation
# Conflicts: # build.gradle # gradle-scripts/spotless.gradle
…2-product-sync-and-tests
Codecov Report
@@ Coverage Diff @@
## java-sdk-v2-product-reference-resolver #947 +/- ##
============================================================================
- Coverage 96.53% 95.26% -1.27%
- Complexity 3755 3921 +166
============================================================================
Files 328 339 +11
Lines 10703 11366 +663
Branches 633 688 +55
============================================================================
+ Hits 10332 10828 +496
- Misses 271 424 +153
- Partials 100 114 +14
... and 7 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
...st/java/com/commercetools/sync/integration/sdk2/ctpprojectsource/products/ProductSyncIT.java
Outdated
Show resolved
Hide resolved
…dk2/ctpprojectsource/products/ProductSyncIT.java Co-authored-by: Sarah Lander <70885646+salander85@users.noreply.github.com>
Summary
Main changes: https://github.com/commercetools/commercetools-sync-java/pull/947/files#diff-98a7b2c5b9b1ea946cc5fc3ded767a6a5395f36a81b18a8017f24bea79fd0e14, https://github.com/commercetools/commercetools-sync-java/pull/947/files#diff-d8e12c9e202ba3b3ac24469897c21c425021fa437481fddc3004f8f62927672f
Errors fixed in this PR
The problem was when using ResourceIdentifier we can set either
key
orid
, but not both. dd113f1When
categoryOrderHints
is an empty object in the product, it is represented as null in Java object. Therefore we have to check to avoid NPE.I renamed the method
createObjectFromResource
toreadObjectFromResource
. Even though the namecreateObjectFromResource
might be slightly better, it was impossible for me to find this method by name when I'm migrating the old code. In the old sdk we have a method calledreadObjectFromResource
. So whenever I migrate the code, I expect a method with the same name exists for sdk v2. Lesson learnt: always keep the same name for the common methods so that they're easy to find.Problem I could not fix
In the old test there was an integration test that verifies that in the method
ProductSync.fetchAndUpdate()
whenever sync fails to update with CT error, the sync would fail with a correct error message etc. This was achieved in the past mocking the execute method with ProductUpdate params. The other methods that fetches from CT were not mocked and were really calling CT. This is not possible anymore with the new sdk v2. To update a product now we have to call sth likeproducts().update().execute()
. So that means we have to mockproducts()
,update()
andexecute()
. However, when we mockproducts()
, we also return the mock for all previous fetches that uses this endpoint in the sync (e.g.products().withKey().get().execute()
), so we have to mock them too. So these kind of tests where you mock only the calls you need and the rest are real method calls are not possible anymore.The rest I had to adapt to ProductSync.java class and its integration tests working. In tests I changed create methods to ensure methods because they were failing too much with Concurrency Exception. Afterwards the tests were not failing anymore.