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
Refactoring org.apache.cloudstack.network.tungsten.service #8098
Refactoring org.apache.cloudstack.network.tungsten.service #8098
Conversation
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
@blueorangutan package |
@harikrishna-patnala a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report
@@ Coverage Diff @@
## main #8098 +/- ##
============================================
+ Coverage 28.15% 29.18% +1.02%
- Complexity 29181 31003 +1822
============================================
Files 5111 5181 +70
Lines 360669 365208 +4539
Branches 52700 53420 +720
============================================
+ Hits 101562 106581 +5019
+ Misses 245113 244022 -1091
- Partials 13994 14605 +611
Flags with carried forward coverage won't be shown. Click here to find out more. see 428 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
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.
good change, some build error though. I added some suggestions as well.
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
|
||
public class CreateMockTungstenAnswer { |
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.
would a more appropriate name be MockTungstenAnswerFactory
?
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.
Good suggestion, I've renamed CreateMockTungstenAnswer
to MockTungstenAnswerFactory
and recommitted.
...n/src/test/java/org/apache/cloudstack/network/tungsten/service/CreateMockTungstenAnswer.java
Outdated
Show resolved
Hide resolved
...n/src/test/java/org/apache/cloudstack/network/tungsten/service/CreateMockTungstenAnswer.java
Outdated
Show resolved
Hide resolved
not sure if the build errors are related to this PR |
Great suggestions, thanks a lot! Co-authored-by: dahn <daan.hoogland@gmail.com>
…work.tungsten.service
I found the error reason; the parameter used |
@blueorangutan package |
@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 7417 |
it still doesn't compile @gzhao9 , can you have a look? |
This is because the most recent update involved a file that I had committed. I've made the update and made sure it works on my end. |
@blueorangutan package |
@DaanHoogland a [LL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
@gzhao9 have a look at https://github.com/apache/cloudstack/actions/runs/6560826849/job/17824151994?pr=8098#step:6:9444 it seems you have some line endings mixed (\n instead of \r?) |
Packaging result [LL]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6208 |
I'm done replacing. |
@blueorangutan package |
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
according to https://github.com/apache/cloudstack/actions/runs/6564274866/job/17850770581?pr=8098#step:6:9439 you still have mixed line endings @gzhao9 . |
I'll keep trying to deal with it. BTW I was very confused. Another PR I submitted did not have this formatting problem. |
I don´t work on windows recently, but I remember a tool called d2u or dos2unix. it can do the conversions to and from. nowadays there is also the use of a VM that could help ;) |
…ice' of https://github.com/gzhao9/cloudstack into refactoring-org.apache.cloudstack.network.tungsten.service
I tried another update to fix the end of line issue. |
@DaanHoogland To avoid also having the ending problem. I submitted two more PRs #8137 and #8139. also with similar idea to solve the duplicate creation problem of mock. If you are interested you can take a look. |
...en/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenFabricUtilsTest.java
Outdated
Show resolved
Hide resolved
...en/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenServiceImplTest.java
Outdated
Show resolved
Hide resolved
...ngsten/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenElementTest.java
Outdated
Show resolved
Hide resolved
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.
clgtm
unit tests only , GHA should suffice. |
* Refactoring reduces mock cloning of TungstenAnswer * Apply suggestions from code review Great suggestions, thanks a lot! Co-authored-by: dahn <daan.hoogland@gmail.com> * Rename CreateMockTungstenAnswer to MockTungstenAnswerFactory * Updated parameter to camel case. * Revised in accordance with the latest update * Replace all `\r` with `\n`. * Replace all \r with \n. * temp for re-uploading * reupdate * update line ending * update ling ending * Add static methods to avoid duplicate creation of new --------- Co-authored-by: dahn <daan.hoogland@gmail.com>
…pache#8098)" This reverts commit eeff89d.
Description
This PR addresses the issue mentioned in #8087 by refactoring the test suites in three classes:
TungstenElementTest.java
,TungstenFabricUtilsTest.java
, andTungstenServiceImplTest.java
. The issue involves redundant mock object creations forTungstenAnswer
in these test suites.Proposed Changes
I propose refactoring the test code to eliminate redundancy and improve maintainability. Specifically, I suggest creating a reusable
CreateMockTungstenAnswer
class for creatingTungstenAnswer
mock objects with consistent stub behavior.Benefits of the Proposed Refactoring:
TungstenAnswer
mock creation needs to be modified in the future, the developer only needs to modify one time inCreateMockTungstenAnswer
instead of 151 places of duplicate mock creation in 3 testclasses.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?