Skip to content

Commit f5ff311

Browse files
stephenxsstephenxs
committed
Fix review comments
Signed-off-by: Stephen Sun <[email protected]>
1 parent 60e5d39 commit f5ff311

File tree

2 files changed

+35
-37
lines changed

2 files changed

+35
-37
lines changed

‎sfputil/main.py

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,7 @@ def is_sfp_present(port_name):
292292
return bool(presence)
293293

294294

295-
def is_rj45_port_from_api(port_name):
295+
def is_port_type_rj45(port_name):
296296
physical_port = logical_port_to_physical_port_index(port_name)
297297

298298
try:
@@ -302,12 +302,6 @@ def is_rj45_port_from_api(port_name):
302302
pass
303303

304304
return False
305-
306-
307-
def skip_if_port_is_rj45(port_name):
308-
if is_rj45_port_from_api(port_name):
309-
click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name))
310-
sys.exit(EXIT_FAIL)
311305
# ========================== Methods for formatting output ==========================
312306

313307
# Convert dict values to cli output string
@@ -649,7 +643,7 @@ def eeprom(port, dump_dom, namespace):
649643
for physical_port in physical_port_list:
650644
port_name = get_physical_port_name(logical_port_name, i, ganged)
651645

652-
if is_rj45_port_from_api(port_name):
646+
if is_port_type_rj45(port_name):
653647
output += "{}: SFP EEPROM is not applicable for RJ45 port\n".format(port_name)
654648
output += '\n'
655649
continue
@@ -808,7 +802,7 @@ def fetch_error_status_from_platform_api(port):
808802
physical_port_list = logical_port_name_to_physical_port_list(logical_port_name)
809803
port_name = get_physical_port_name(logical_port_name, 1, False)
810804

811-
if is_rj45_port_from_api(logical_port_name):
805+
if is_port_type_rj45(logical_port_name):
812806
output.append([port_name, "N/A"])
813807
else:
814808
output.append([port_name, output_dict.get(physical_port_list[0])])
@@ -834,7 +828,7 @@ def fetch_error_status_from_state_db(port, state_db):
834828
sorted_ports = natsort.natsorted(status)
835829
output = []
836830
for port in sorted_ports:
837-
if is_rj45_port_from_api(port):
831+
if is_port_type_rj45(port):
838832
description = "N/A"
839833
else:
840834
statestring = status[port].get('status')
@@ -910,7 +904,7 @@ def lpmode(port):
910904
click.echo("Error: No physical ports found for logical port '{}'".format(logical_port_name))
911905
return
912906

913-
if is_rj45_port_from_api(logical_port_name):
907+
if is_port_type_rj45(logical_port_name):
914908
output_table.append([logical_port_name, "N/A"])
915909
else:
916910
if len(physical_port_list) > 1:
@@ -953,7 +947,7 @@ def fwversion(port_name):
953947
physical_port = logical_port_to_physical_port_index(port_name)
954948
sfp = platform_chassis.get_sfp(physical_port)
955949

956-
if is_rj45_port_from_api(port_name):
950+
if is_port_type_rj45(port_name):
957951
click.echo("Show firmware version is not applicable for RJ45 port {}.".format(port_name))
958952
sys.exit(EXIT_FAIL)
959953

@@ -992,7 +986,7 @@ def set_lpmode(logical_port, enable):
992986
click.echo("Error: No physical ports found for logical port '{}'".format(logical_port))
993987
return
994988

995-
if is_rj45_port_from_api(logical_port):
989+
if is_port_type_rj45(logical_port):
996990
click.echo("{} low-power mode is not applicable for RJ45 port {}.".format("Enabling" if enable else "Disabling", logical_port))
997991
sys.exit(EXIT_FAIL)
998992

@@ -1052,7 +1046,7 @@ def reset(port_name):
10521046
click.echo("Error: No physical ports found for logical port '{}'".format(port_name))
10531047
return
10541048

1055-
if is_rj45_port_from_api(port_name):
1049+
if is_port_type_rj45(port_name):
10561050
click.echo("Reset is not applicable for RJ45 port {}.".format(port_name))
10571051
sys.exit(EXIT_FAIL)
10581052

@@ -1217,7 +1211,9 @@ def download_firmware(port_name, filepath):
12171211
def run(port_name, mode):
12181212
"""Run the firmware with default mode=1"""
12191213

1220-
skip_if_port_is_rj45(port_name)
1214+
if is_port_type_rj45(port_name):
1215+
click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name))
1216+
sys.exit(EXIT_FAIL)
12211217

12221218
if not is_sfp_present(port_name):
12231219
click.echo("{}: SFP EEPROM not detected\n".format(port_name))
@@ -1236,7 +1232,9 @@ def run(port_name, mode):
12361232
def commit(port_name):
12371233
"""Commit the running firmware"""
12381234

1239-
skip_if_port_is_rj45(port_name)
1235+
if is_port_type_rj45(port_name):
1236+
click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name))
1237+
sys.exit(EXIT_FAIL)
12401238

12411239
if not is_sfp_present(port_name):
12421240
click.echo("{}: SFP EEPROM not detected\n".format(port_name))
@@ -1258,7 +1256,9 @@ def upgrade(port_name, filepath):
12581256

12591257
physical_port = logical_port_to_physical_port_index(port_name)
12601258

1261-
skip_if_port_is_rj45(port_name)
1259+
if is_port_type_rj45(port_name):
1260+
click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name))
1261+
sys.exit(EXIT_FAIL)
12621262

12631263
if not is_sfp_present(port_name):
12641264
click.echo("{}: SFP EEPROM not detected\n".format(port_name))
@@ -1294,7 +1294,9 @@ def upgrade(port_name, filepath):
12941294
def download(port_name, filepath):
12951295
"""Download firmware on the transceiver"""
12961296

1297-
skip_if_port_is_rj45(port_name)
1297+
if is_port_type_rj45(port_name):
1298+
click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name))
1299+
sys.exit(EXIT_FAIL)
12981300

12991301
if not is_sfp_present(port_name):
13001302
click.echo("{}: SFP EEPROM not detected\n".format(port_name))
@@ -1320,7 +1322,9 @@ def unlock(port_name, password):
13201322
physical_port = logical_port_to_physical_port_index(port_name)
13211323
sfp = platform_chassis.get_sfp(physical_port)
13221324

1323-
skip_if_port_is_rj45(port_name)
1325+
if is_port_type_rj45(port_name):
1326+
click.echo("This functionality is not applicable for RJ45 port {}.".format(port_name))
1327+
sys.exit(EXIT_FAIL)
13241328

13251329
if not is_sfp_present(port_name):
13261330
click.echo("{}: SFP EEPROM not detected\n".format(port_name))

‎tests/sfputil_test.py

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ def test_version(self):
268268
result = runner.invoke(sfputil.cli.commands['version'], [])
269269
assert result.output.rstrip() == 'sfputil version {}'.format(sfputil.VERSION)
270270

271-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=False))
271+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=False))
272272
def test_error_status_from_db(self):
273273
db = Db()
274274
expected_output = [['Ethernet0', 'Blocking Error|High temperature'],
@@ -285,7 +285,7 @@ def test_error_status_from_db(self):
285285
output = sfputil.fetch_error_status_from_state_db('Ethernet0', db.db)
286286
assert output == expected_output_ethernet0
287287

288-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
288+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
289289
def test_error_status_from_db_RJ45(self):
290290
db = Db()
291291
expected_output = [['Ethernet0', 'N/A'],
@@ -304,7 +304,7 @@ def test_error_status_from_db_RJ45(self):
304304

305305
@patch('sfputil.main.logical_port_name_to_physical_port_list', MagicMock(return_value=[1]))
306306
@patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
307-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=False))
307+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=False))
308308
@patch('subprocess.check_output', MagicMock(return_value="['0:OK']"))
309309
def test_fetch_error_status_from_platform_api(self):
310310
output = sfputil.fetch_error_status_from_platform_api('Ethernet0')
@@ -313,7 +313,7 @@ def test_fetch_error_status_from_platform_api(self):
313313
@patch('sfputil.main.logical_port_name_to_physical_port_list', MagicMock(return_value=[1]))
314314
@patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
315315
@patch('subprocess.check_output', MagicMock(return_value="['0:OK']"))
316-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
316+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
317317
def test_fetch_error_status_from_platform_api_RJ45(self):
318318
output = sfputil.fetch_error_status_from_platform_api('Ethernet0')
319319
assert output == [['Ethernet0', 'N/A']]
@@ -407,7 +407,7 @@ def test_show_lpmode(self, mock_chassis):
407407
@patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
408408
@patch('sfputil.main.logical_port_name_to_physical_port_list', MagicMock(return_value=[1]))
409409
@patch('sfputil.main.platform_sfputil', MagicMock(is_logical_port=MagicMock(return_value=1)))
410-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
410+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
411411
def test_show_eeprom_RJ45(self, mock_chassis):
412412
mock_sfp = MagicMock()
413413
mock_api = MagicMock()
@@ -418,14 +418,8 @@ def test_show_eeprom_RJ45(self, mock_chassis):
418418
expected_output = "Ethernet16: SFP EEPROM is not applicable for RJ45 port\n\n\n"
419419
assert result.output == expected_output
420420

421-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
422-
@patch('sys.exit', MagicMock(return_value=EXIT_FAIL))
423-
def test_skip_if_port_is_rj45(self):
424-
result = sfputil.skip_if_port_is_rj45('Ethernet0')
425-
assert result == None
426-
427421
@patch('sfputil.main.logical_port_name_to_physical_port_list', MagicMock(return_value=1))
428-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
422+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
429423
@patch('sfputil.main.platform_sfputil', MagicMock(is_logical_port=MagicMock(return_value=1)))
430424
def test_lpmode_set(self):
431425
runner = CliRunner()
@@ -434,7 +428,7 @@ def test_lpmode_set(self):
434428
assert result.exit_code == EXIT_FAIL
435429

436430
@patch('sfputil.main.logical_port_name_to_physical_port_list', MagicMock(return_value=1))
437-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
431+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
438432
@patch('sfputil.main.platform_sfputil', MagicMock(is_logical_port=MagicMock(return_value=1)))
439433
def test_reset_RJ45(self):
440434
runner = CliRunner()
@@ -457,7 +451,7 @@ def test_unlock_firmware(self, mock_chassis):
457451

458452
@patch('sfputil.main.platform_chassis')
459453
@patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
460-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
454+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
461455
def test_show_fwversion_Rj45(self, mock_chassis):
462456
mock_sfp = MagicMock()
463457
mock_api = MagicMock()
@@ -494,23 +488,23 @@ def test_commit_firmwre(self, mock_chassis):
494488
assert status == 1
495489

496490
@patch('sfputil.main.is_sfp_present', MagicMock(return_value=True))
497-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
491+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
498492
def test_firmware_run_RJ45(self):
499493
runner = CliRunner()
500494
result = runner.invoke(sfputil.cli.commands['firmware'].commands['run'], ["--mode", "0", "Ethernet0"])
501495
assert result.output == 'This functionality is not applicable for RJ45 port Ethernet0.\n'
502496
assert result.exit_code == EXIT_FAIL
503497

504498
@patch('sfputil.main.is_sfp_present', MagicMock(return_value=True))
505-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
499+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
506500
def test_firmware_commit_RJ45(self):
507501
runner = CliRunner()
508502
result = runner.invoke(sfputil.cli.commands['firmware'].commands['commit'], ["Ethernet0"])
509503
assert result.output == 'This functionality is not applicable for RJ45 port Ethernet0.\n'
510504
assert result.exit_code == EXIT_FAIL
511505

512506
@patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
513-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
507+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
514508
@patch('sfputil.main.is_sfp_present', MagicMock(return_value=1))
515509
def test_firmware_upgrade_RJ45(self):
516510
runner = CliRunner()
@@ -519,7 +513,7 @@ def test_firmware_upgrade_RJ45(self):
519513
assert result.exit_code == EXIT_FAIL
520514

521515
@patch('sfputil.main.logical_port_to_physical_port_index', MagicMock(return_value=1))
522-
@patch('sfputil.main.is_rj45_port_from_api', MagicMock(return_value=True))
516+
@patch('sfputil.main.is_port_type_rj45', MagicMock(return_value=True))
523517
@patch('sfputil.main.is_sfp_present', MagicMock(return_value=1))
524518
def test_firmware_download_RJ45(self):
525519
runner = CliRunner()

0 commit comments

Comments
 (0)