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.

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

Jump to bottom

[test_fgnhg] Test enhancement for link operational change and matchmode changes to fine grained ECMP #2670

Merged
merged 8 commits into from
Apr 6, 2021
Merged

[test_fgnhg] Test enhancement for link operational change and matchmode changes to fine grained ECMP #2670

merged 8 commits into from
Apr 6, 2021

Conversation

anish-n
Copy link
Contributor

@anish-n anish-n commented Dec 15, 2020 •
edited
Loading

Description of PR

Summary:
Enhance test_fgnhg with link operational change testing.

Type of change

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

Approach

What is the motivation for this PR?

HLD update: sonic-net/SONiC#693

How did you do it?

Change link operational state in the test and validate that ECMP adjusts to account for it

How did you verify/test it?

By running the test via pytest/ptf

Any platform specific information?

Applicable to all platforms which support fine grained ecmp

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

Documentation

sonic-net/SONiC#693

@lgtm-com
Copy link

lgtm-com bot commented Dec 15, 2020

This pull request introduces 2 alerts when merging 63034b2 into c86b445 - view on LGTM.com

new alerts:

  • 2 for Result of integer division may be truncated

@lgtm-com
Copy link

lgtm-com bot commented Feb 1, 2021

This pull request introduces 2 alerts when merging 6594039 into b901773 - view on LGTM.com

new alerts:

  • 2 for Result of integer division may be truncated

@anish-n anish-n requested a review from a team as a code owner February 18, 2021 04:54
@lgtm-com
Copy link

lgtm-com bot commented Feb 18, 2021

This pull request introduces 6 alerts when merging 298a5bf into a0836c8 - view on LGTM.com

new alerts:

  • 5 for Result of integer division may be truncated
  • 1 for Wrong number of arguments in a call

@anish-n anish-n changed the title [test_fgnhg] Link operational change testing [test_fgnhg] Test enhancement for link operational change and matchmode changes to fine grained ECMP Feb 22, 2021
@anish-n
Copy link
Contributor Author

anish-n commented Mar 3, 2021

@nazariig please can you help review?

@anish-n
Add support for warm boot in the ptf test, warm boot is not triggered…
337dded 
… in the pytest currently, it is planned to be added to pytest in a future PR
abdosi
abdosi requested changes Mar 24, 2021
View reviewed changes
tests/ecmp/test_fgnhg.py Outdated Show resolved Hide resolved
tests/ecmp/test_fgnhg.py Show resolved Hide resolved
tests/ecmp/test_fgnhg.py Show resolved Hide resolved
tests/ecmp/test_fgnhg.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2021

This pull request introduces 12 alerts when merging 013ce50 into 1141ee7 - view on LGTM.com

new alerts:

  • 5 for Result of integer division may be truncated
  • 4 for Testing equality to None
  • 2 for Unused local variable
  • 1 for Wrong number of arguments in a call

wangxin
wangxin reviewed Mar 31, 2021
View reviewed changes
Copy link
Collaborator

@wangxin wangxin left a comment

Choose a reason for hiding this comment

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

Can you also fix the LGTM alerts?

tests/ecmp/test_fgnhg.py Outdated Show resolved Hide resolved
tests/ecmp/test_fgnhg.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Apr 1, 2021

This pull request introduces 6 alerts when merging 5097deb into a2a435c - view on LGTM.com

new alerts:

  • 5 for Result of integer division may be truncated
  • 1 for Unused import

@anish-n
Copy link
Contributor Author

anish-n commented Apr 1, 2021

Can you also fix the LGTM alerts?

Fixed LGTM errors, the remaining one on result of integer division may be truncated is fine from the test logic perspective and may be ignored.

@anish-n anish-n requested a review from abdosi April 1, 2021 00:15
@lgtm-com
Copy link

lgtm-com bot commented Apr 1, 2021

This pull request introduces 5 alerts when merging e2c17ba into a2a435c - view on LGTM.com

new alerts:

  • 5 for Result of integer division may be truncated

@abdosi
Copy link
Contributor

abdosi commented Apr 1, 2021

can you fix LGTM ?

@anish-n
Copy link
Contributor Author

anish-n commented Apr 1, 2021 •
edited by abdosi
Loading

can you fix LGTM ?

Have fixed most LGTM errors, the remaining one on result of integer division may be truncated is fine from the test logic perspective and can be ignored?

it can't be fix ?

@anish-n
Copy link
Contributor Author

anish-n commented Apr 6, 2021

@abdosi , @wangxin can you please help merge the PR if it is good to go?

@abdosi abdosi merged commit f8a5a6b into sonic-net:master Apr 6, 2021
@JibinBao JibinBao mentioned this pull request Apr 13, 2021
Open
saravanansv pushed a commit to saravanansv/sonic-mgmt that referenced this pull request May 6, 2021
@anish-n @saravanansv
[test_fgnhg] Test enhancement for link operational change and matchmo…
7ce0117 
…de changes to fine grained ECMP (sonic-net#2670)

Enhance test_fgnhg with link operational change testing.
vmittal-msft pushed a commit to vmittal-msft/sonic-mgmt that referenced this pull request Sep 28, 2021
@anish-n @vmittal-msft
[test_fgnhg] Test enhancement for link operational change and matchmo…
4f20f8a 
…de changes to fine grained ECMP (sonic-net#2670)

Enhance test_fgnhg with link operational change testing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vivekrnv vivekrnv vivekrnv left review comments

@wangxin wangxin wangxin approved these changes

@kktheballer kktheballer kktheballer approved these changes

@nazariig nazariig nazariig approved these changes

@abdosi abdosi abdosi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@anish-n @abdosi @wangxin @kktheballer @vivekrnv @nazariig