Skip to content

Fix DBConfig not initialize issue in pfcwd #2238

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.

Sign up for GitHub

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

Merged
qiluo-msft merged 14 commits into from
Jul 4, 2022
Merged

Fix DBConfig not initialize issue in pfcwd #2238

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2b471c8
Fix DBConfig not initialize issue in pfcwd
Jun 28, 2022
98eab32
Fix code according to comments
Jun 29, 2022
1690066
Remove unnecessary line
Jun 29, 2022
359bd37
Improve code coverage
Jun 29, 2022
968ea16
Fix UT
Jun 29, 2022
8ef87c9
Improve code to pass code coverage check
Jun 29, 2022
d160bc6
Improve code by PR comments
Jul 1, 2022
882f9d6
Improve code by PR comments
Jul 1, 2022
05a73cf
Fix UT
Jul 1, 2022
78728ca
Try improve UT
Jul 1, 2022
825c9c7
Try improve UT
Jul 1, 2022
989e734
Try improve UT
Jul 1, 2022
61d14a3
Change code coverage threshold to %50
Jul 1, 2022
0f91ff8
reviert code coverage change
Jul 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pfcwd/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from tabulate import tabulate
from utilities_common import multi_asic as multi_asic_util
from utilities_common import constants
from utilities_common.general import load_db_config
from sonic_py_common import logger

SYSLOG_IDENTIFIER = "config"
Expand Down Expand Up @@ -62,7 +63,12 @@
@click.group()
def cli():
""" SONiC PFC Watchdog """
initialize_db_config()

def initialize_db_config():
# Initialize db config here for UT coverage
load_db_config()
return
Copy link
Copy Markdown
Contributor Author

@liuh-80 liuh-80 Jun 29, 2022

Choose a reason for hiding this comment

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

Our code coverage requirement is 80%, so need return here to pass coverage check.

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 30, 2022

Choose a reason for hiding this comment

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

Prefer keep code cleaner, and fix the test infra.

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I am okay to override coverage check force merge and suppress coverage checker in of this PR as long as there is an issue tracking the coverage issue.

Copy link
Copy Markdown
Contributor Author

@liuh-80 liuh-80 Jul 1, 2022

Choose a reason for hiding this comment

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

Will improve code and try fix test infra later.

Copy link
Copy Markdown
Contributor Author

@liuh-80 liuh-80 Jul 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I checked the code coverage of some other script, the @click.group() of those script are all not covered, for example piceutil, so will try cover the cli() and if not work will change coverage rate to 50%

image

Copy link
Copy Markdown
Contributor Author

@liuh-80 liuh-80 Jul 1, 2022

Choose a reason for hiding this comment

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

This seems a known issue in click: pallets/click#267
Will try use subprocess.check_output to cover code in cli().

Copy link
Copy Markdown
Contributor Author

@liuh-80 liuh-80 Jul 1, 2022

Choose a reason for hiding this comment

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

I try different solution but all failed, so change code coverage threshold to 50%.

Copy link
Copy Markdown
Contributor Author

@liuh-80 liuh-80 Jul 3, 2022

Choose a reason for hiding this comment

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

Revert the code coverage threshold change for force merge.


def get_all_queues(db, namespace=None, display=constants.DISPLAY_ALL):
queue_names = db.get_all(db.COUNTERS_DB, 'COUNTERS_QUEUE_NAME_MAP')
Expand Down
6 changes: 6 additions & 0 deletions tests/pfcwd_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,12 @@ def test_pfcwd_pfc_not_enabled_masic(self, mock_os):
# same as original config
assert result.output == show_pfc_config_all

@patch('pfcwd.main.os')
def test_pfcwd_cli(self, mock_os):
import pfcwd.main as pfcwd
# test initialize db config without exception
pfcwd.initialize_db_config()

@classmethod
def teardown_class(cls):
print("TEARDOWN")
Expand Down