Skip to content
This repository has been archived by the owner on Dec 20, 2023. It is now read-only.

Feature/wdm next refactor #341

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

allenshihgoog
Copy link

Major refactor improvements on wdmNext automation infrastructure.
This is PHASE 1 + PHASE 2 of the design

PHASE 1:

  • Refactor all parameter options and default values to const file WeaveWdmNextOptions.py.
  • Options now split between test/client/server
  • Naming of options now consistent with test-app CLI arguments.
  • Each parameter option is documented for its use case
  • Improved processing of client/server options into optimized loop reducing 130 lines -> 20 lines

PHASE 2:

  • Refactor ALL test parameters for 90+ test files into weave_wdm_next_test_params.json keyed by the test name and then test/client/server parameters. All test parameters were parsed using this script
  • All 90 tests now will read from json to run their test params.

Main files to review:

  1. weave_wdm_next_test_base.py
  2. WeaveWdmNext.py
  3. WeaveWdmNextOptions.py
  4. weave_wdm_next_test_params.json

All functions in these files are now documented on what they do, what their parameters are, and what they return.

PHASE 3 will be next to group test cases so that they can share the same setup and teardown of weave topology to significantly increase testing speed.

NOTE:
Changes for wdmNext service tests are NOT here. I need to work with @yunhanw-google to figure out how to run them.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@gerickson
Copy link
Contributor

If this is the "base" of what becomes normal, is there a need to name the files as part of this "Next" and "next"? I'd recommend just dropping those naming decorations.

Copy link
Contributor

@gerickson gerickson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At minimum, please see my naming feedback. Beyond that, I have a few minor comments in the design document.

@allenshihgoog allenshihgoog force-pushed the feature/wdmNextRefactor branch 2 times, most recently from f978568 to 0f9f23c Compare August 27, 2019 00:11
@codecov-io
Copy link

codecov-io commented Aug 27, 2019

Codecov Report

Merging #341 into master will decrease coverage by 1.7%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
- Coverage   54.46%   52.76%   -1.71%     
==========================================
  Files         340      326      -14     
  Lines       58425    56047    -2378     
==========================================
- Hits        31821    29573    -2248     
+ Misses      26604    26474     -130
Impacted Files Coverage Δ
src/inet/InetFaultInjection.cpp 0% <0%> (-100%) ⬇️
src/test-apps/ToolCommon.h 0% <0%> (-100%) ⬇️
src/test-apps/TestInetEndPoint.cpp 1.8% <0%> (-96.8%) ⬇️
src/inet/InetUtils.cpp 0% <0%> (-95.35%) ⬇️
src/inet/InetInterface.cpp 0% <0%> (-65.1%) ⬇️
src/inet/InetLayer.cpp 4.01% <0%> (-63.77%) ⬇️
src/lib/profiles/security/WeaveAccessToken.cpp 0% <0%> (-48.15%) ⬇️
src/inet/DNSResolver.cpp 0% <0%> (-47.56%) ⬇️
src/inet/UDPEndPoint.cpp 0% <0%> (-43.16%) ⬇️
src/lib/profiles/security/WeaveCert.h 61.53% <0%> (-38.47%) ⬇️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a95ab81...ea7b7bc. Read the comment docs.

@allenshihgoog
Copy link
Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@robszewczyk
Copy link
Member

If this is the "base" of what becomes normal, is there a need to name the files as part of this "Next" and "next"? I'd recommend just dropping those naming decorations.

I would suggest we decouple concerns: lets refactor the tests and then rename them; at the moment, this PR still maintains the 1-1 correspondence between the previous tests and the new ones; for traceability purposes, it may be cleaner to have at least separate commits but separate PRs may be more appropriate given the size of the change.

@allenshihgoog allenshihgoog force-pushed the feature/wdmNextRefactor branch from cb5d127 to cff7dc9 Compare September 3, 2019 17:46
@allenshihgoog
Copy link
Author

Here are logs of Jenkins job using my branch to run the WDM service tests. Logs indicate the same results as running off of master branch that CSI team uses to pull data from.
Note: Not all of the tests "pass", but it seems CSI team pulls other data from the runs which is sufficient for them to consider it passing.

https://paste.googleplex.com/4976942140882944
Jenkins log link was removed because they run many times a day and don't keep older runs.

@allenshihgoog allenshihgoog force-pushed the feature/wdmNextRefactor branch from cff7dc9 to ea7b7bc Compare November 7, 2019 00:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants