Refactoring org.apache.cloudstack.network.tungsten.service by gzhao9 · Pull Request #8098 · apache/cloudstack

gzhao9 · 2023-10-15T22:43:57Z

Description

This PR addresses the issue mentioned in #8087 by refactoring the test suites in three classes: TungstenElementTest.java, TungstenFabricUtilsTest.java, and TungstenServiceImplTest.java. The issue involves redundant mock object creations for TungstenAnswer 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 creating TungstenAnswer mock objects with consistent stub behavior.

Benefits of the Proposed Refactoring:

As a result, we can reduce 321 lines of code, which makes the test cases smaller, easier to understand, and better to maintain.
After refactoring, if the TungstenAnswer mock creation needs to be modified in the future, the developer only needs to modify one time in CreateMockTungstenAnswer instead of 151 places of duplicate mock creation in 3 testclasses.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

…work.tungsten.service

boring-cyborg · 2023-10-15T22:44:01Z

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)
Here are some useful points:

In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
Be sure to read the CloudStack Coding Conventions.
Apache CloudStack is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@cloudstack.apache.org (https://cloudstack.apache.org/mailing-lists.html)
Slack: https://apachecloudstack.slack.com/

harikrishna-patnala · 2023-10-16T06:59:03Z

@blueorangutan package

blueorangutan · 2023-10-16T07:00:03Z

@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 · 2023-10-16T07:09:01Z

Codecov Report

Merging #8098 (5deb5dd) into main (543c54c) will increase coverage by 1.02%.
Report is 25 commits behind head on main.
The diff coverage is n/a.

@@             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

Flag	Coverage Δ
simulator-marvin-tests	`25.14% <ø> (+1.27%)`	⬆️
uitests	`4.50% <ø> (-0.30%)`	⬇️
unit-tests	`14.80% <ø> (+0.28%)`	⬆️

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!

DaanHoogland

good change, some build error though. I added some suggestions as well.

DaanHoogland · 2023-10-16T07:19:49Z

...n/src/test/java/org/apache/cloudstack/network/tungsten/service/CreateMockTungstenAnswer.java

+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class CreateMockTungstenAnswer {


would a more appropriate name be MockTungstenAnswerFactory?

Good suggestion, I've renamed CreateMockTungstenAnswer to MockTungstenAnswerFactory and recommitted.

...n/src/test/java/org/apache/cloudstack/network/tungsten/service/CreateMockTungstenAnswer.java

DaanHoogland · 2023-10-16T07:25:45Z

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

gzhao9 · 2023-10-17T04:36:18Z

not sure if the build errors are related to this PR

I found the error reason; the parameter used Result, which doesn't follow camel case naming. I've made an update.

DaanHoogland · 2023-10-18T08:01:41Z

@blueorangutan package

blueorangutan · 2023-10-18T08:02:03Z

@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.

blueorangutan · 2023-10-18T09:15:17Z

Packaging result [SF]: ✖️ el7 ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 7417

DaanHoogland · 2023-10-18T10:17:58Z

not sure if the build errors are related to this PR

I found the error reason; the parameter used Result, which doesn't follow camel case naming. I've made an update.

it still doesn't compile @gzhao9 , can you have a look?

…work.tungsten.service

gzhao9 · 2023-10-18T12:31:58Z

not sure if the build errors are related to this PR

I found the error reason; the parameter used Result, which doesn't follow camel case naming. I've made an update.

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.

DaanHoogland · 2023-10-18T14:28:15Z

@blueorangutan package

blueorangutan · 2023-10-18T14:30:02Z

@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.

DaanHoogland · 2023-10-18T14:41:43Z

@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?)

blueorangutan · 2023-10-18T15:56:39Z

Packaging result [LL]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6208

gzhao9 · 2023-10-18T17:18:27Z

@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?)

I'm done replacing.

DaanHoogland · 2023-10-19T07:50:58Z

@blueorangutan package

blueorangutan · 2023-10-19T07:52:04Z

@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.

DaanHoogland · 2023-10-19T08:20:09Z

according to https://github.com/apache/cloudstack/actions/runs/6564274866/job/17850770581?pr=8098#step:6:9439 you still have mixed line endings @gzhao9 .

gzhao9 · 2023-10-19T12:57:40Z

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.
Also, I'm curious how you guys generally deal with this issue on Windows.

DaanHoogland · 2023-10-20T07:18:18Z

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. Also, I'm curious how you guys generally deal with this issue on Windows.

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 ;)

…ngsten.service

…ice' of https://github.com/gzhao9/cloudstack into refactoring-org.apache.cloudstack.network.tungsten.service

gzhao9 · 2023-10-24T16:02:15Z

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. Also, I'm curious how you guys generally deal with this issue on Windows.

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 ;)

I tried another update to fix the end of line issue.

gzhao9 · 2023-10-24T18:11:07Z

@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

...en/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenServiceImplTest.java

...ngsten/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenElementTest.java

DaanHoogland

clgtm

DaanHoogland · 2023-11-03T08:35:31Z

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.

gzhao9 added 2 commits October 11, 2023 17:41

Refactoring reduces mock cloning of TungstenAnswer

853a49c

Merge branch 'apache:main' into refactoring-org.apache.cloudstack.net…

4d92bd0

…work.tungsten.service

boring-cyborg bot added the component:networking label Oct 15, 2023

DaanHoogland reviewed Oct 16, 2023

View reviewed changes

DaanHoogland closed this Oct 16, 2023

DaanHoogland reopened this Oct 16, 2023

gzhao9 and others added 4 commits October 16, 2023 12:31

Apply suggestions from code review

38cfd97

Great suggestions, thanks a lot! Co-authored-by: dahn <daan.hoogland@gmail.com>

Rename CreateMockTungstenAnswer to MockTungstenAnswerFactory

dc27321

Merge branch 'apache:main' into refactoring-org.apache.cloudstack.net…

d6bab02

…work.tungsten.service

Updated parameter to camel case.

2c8b1ac

gzhao9 added 2 commits October 18, 2023 07:38

Merge branch 'apache:main' into refactoring-org.apache.cloudstack.net…

fde8fad

…work.tungsten.service

Revised in accordance with the latest update

f525d60

gzhao9 added 2 commits October 18, 2023 13:10

Replace all \r with \n.

526d38c

Replace all \r with \n.

3985310

gzhao9 added 6 commits October 20, 2023 11:01

temp for re-uploading

b613c99

reupdate

1e1c8a2

Merge branch 'main' into refactoring-org.apache.cloudstack.network.tu…

6dd5b88

…ngsten.service

update line ending

69c2905

Merge branch 'refactoring-org.apache.cloudstack.network.tungsten.serv…

0c61bcf

…ice' of https://github.com/gzhao9/cloudstack into refactoring-org.apache.cloudstack.network.tungsten.service

update ling ending

66f5737

gzhao9 mentioned this pull request Oct 24, 2023

refactor MockNetworkVO #8137

Merged

DaanHoogland closed this Oct 25, 2023

DaanHoogland reopened this Oct 25, 2023

DaanHoogland reviewed Oct 25, 2023

View reviewed changes

...en/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenFabricUtilsTest.java Outdated Show resolved Hide resolved

DaanHoogland reviewed Oct 25, 2023

View reviewed changes

...en/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenServiceImplTest.java Outdated Show resolved Hide resolved

DaanHoogland reviewed Oct 25, 2023

View reviewed changes

...ngsten/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenElementTest.java Outdated Show resolved Hide resolved

Add static methods to avoid duplicate creation of new

5deb5dd

gzhao9 requested a review from DaanHoogland November 2, 2023 19:17

DaanHoogland approved these changes Nov 3, 2023

View reviewed changes

DaanHoogland added this to the 4.19.0.0 milestone Nov 3, 2023

DaanHoogland added the component:unit-test label Nov 3, 2023

yadvr approved these changes Nov 3, 2023

View reviewed changes

yadvr merged commit 9e8f591 into apache:main Nov 3, 2023

Dajeong-Park added a commit to Dajeong-Park/ablestack-cloud that referenced this pull request Nov 16, 2023

Revert "Refactoring org.apache.cloudstack.network.tungsten.service (a…

9d98c3b

…pache#8098)" This reverts commit eeff89d.

Conversation

gzhao9 commented Oct 15, 2023

Description

Proposed Changes

Benefits of the Proposed Refactoring:

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?

Uh oh!

boring-cyborg bot commented Oct 15, 2023

Uh oh!

harikrishna-patnala commented Oct 16, 2023

Uh oh!

blueorangutan commented Oct 16, 2023

Uh oh!

codecov bot commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

gzhao9 Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented Oct 16, 2023

Uh oh!

gzhao9 commented Oct 17, 2023

Uh oh!

DaanHoogland commented Oct 18, 2023

Uh oh!

blueorangutan commented Oct 18, 2023

Uh oh!

blueorangutan commented Oct 18, 2023

Uh oh!

DaanHoogland commented Oct 18, 2023

Uh oh!

gzhao9 commented Oct 18, 2023

Uh oh!

DaanHoogland commented Oct 18, 2023

Uh oh!

blueorangutan commented Oct 18, 2023

Uh oh!

DaanHoogland commented Oct 18, 2023

Uh oh!

blueorangutan commented Oct 18, 2023

Uh oh!

gzhao9 commented Oct 18, 2023

Uh oh!

DaanHoogland commented Oct 19, 2023

Uh oh!

blueorangutan commented Oct 19, 2023

Uh oh!

DaanHoogland commented Oct 19, 2023

Uh oh!

gzhao9 commented Oct 19, 2023

Uh oh!

DaanHoogland commented Oct 20, 2023

Uh oh!

gzhao9 commented Oct 24, 2023

Uh oh!

gzhao9 commented Oct 24, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Nov 3, 2023

Uh oh!

codecov bot commented Oct 16, 2023 •

edited

Loading