Skip to content
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

Added skip for test_mgmt_ipv6_only TC if ipv6 mgmt not available #14273

Open
wants to merge 1 commit into
base: 202405
Choose a base branch
from

Conversation

SavchukRomanLv
Copy link
Contributor

Description of PR

Summary:
Fixes failure if DUT does not have ipv6 management address set

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405

Approach

What is the motivation for this PR?

If DUT does not have ipv6 mgmt address to be set - TC fails

How did you do it?

Add pytest.skip if ipv6 mgmt address not set

How did you verify/test it?

Run TC. TC skip

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@SavchukRomanLv
Copy link
Contributor Author

Hi @wangxin @yxieca can you please take a look?

pytest.skip(f"{duthost.hostname} doesn't have available IPv6 Management IP address")

if not ipv6_address[duthost.hostname]:
pytest.skip(f"{duthost.hostname} doesn't have IPv6 Management IP address")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate on why reordering these 2 conditions?

If we do have assigned IPv6 address but not configured on the mgmt port, should this be a failure or skip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @yxieca for me it still setup issue and not TC issue, so I'd prefer skip rather than fail. How do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@SavchukRomanLv I think Ying's question is why you check has_available_ipv6_addr before ipv6_address. Is there a particular reason for changing the order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bingwang-ms - I got you, returned order as it was before, I've accidentally changed it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bingwang-ms I must do additional changes not related to my PR to pass flake issues:
seems like flake also does analysis of f strings:

tests/common/fixtures/duthost_utils.py:846:36: E713 test for membership should be 'not in'
tests/common/fixtures/duthost_utils.py:851:34: E713 test for membership should be 'not in'

should it work in such way?

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants