Skip to content

Converted port_toggle test and continuous_link_flap test to pytest#2389

Merged
yxieca merged 15 commits intosonic-net:masterfrom
OleksandrKozodoi:port_toggle_to_pytest
Nov 5, 2020
Merged

Converted port_toggle test and continuous_link_flap test to pytest#2389
yxieca merged 15 commits intosonic-net:masterfrom
OleksandrKozodoi:port_toggle_to_pytest

Conversation

@OleksandrKozodoi
Copy link
Copy Markdown
Contributor

@OleksandrKozodoi OleksandrKozodoi commented Oct 22, 2020

Description of PR

Summary:

  1. Converted continuous_link_flap test to pytest
  2. Converted port_toggle test to pytest
  3. Changed link_flap test
  4. Added new fixture for test teardown
  5. Changed port_toggle.py module based on port_toggle test

Note:
A few methods of link_flap.py module are useful for cont_link_flap test, so I've changed the structure of the link flap test

Type of change

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

Approach

What is the motivation for this PR?

Migrating tests from ansible to pytest framework

How did you do it?

Converted port_toggle and continuous_link_flap ansible tests to pytest.
Old versions of ansible tests:
port_toggle
continuous_link_flap

How did you verify/test it?

Run tests. Test passed.

link_flap test:
py.test platform_tests/link_flap/test_link_flap.py -rA

port_toggle:
py.test platform_tests/link_flap/test_port_toggle.py -rA

cont_link_flap:
py.test platform_tests/link_flap/test_cont_link_flap.py --orch_cpu_threshold=10 -rA
--orch_cpu_threshold is optional

Any platform specific information?

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

Documentation

@lguohan
Copy link
Copy Markdown
Contributor

lguohan commented Oct 22, 2020

can you provide command line how to run this test?

@OleksandrKozodoi
Copy link
Copy Markdown
Contributor Author

can you provide command line how to run this test?

Sure. Added to Approach block.

@OleksandrKozodoi
Copy link
Copy Markdown
Contributor Author

@yxieca @wangxin Please review

@daall daall requested review from a team, wangxin and yxieca October 26, 2020 18:45
Copy link
Copy Markdown
Contributor

@daall daall left a comment

Choose a reason for hiding this comment

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

The overall approach looks OK to me, but I would recommend going through and using wait_until from common/utilities in places where waiting is necessary (I think I pointed out most of them). It might also be worth considering using this in some places where there is a static 30-60s wait time to improve test stability.

@yxieca
Copy link
Copy Markdown
Collaborator

yxieca commented Oct 28, 2020

@OleksandrKozodoi I am refactoring test_link_flap to let it test parameterized individual ports: #2411. My change will cause some merge conflict with your change. I think we can still share the code between 2 tests as you intended though.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A topo marker must be assigned, or the case will be skipped.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

"dut port {} didn't go down as expected"
The error msg should be "dut port {} didn't go up as expected"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suggest combining pytest_assert with wait_until like
pytest_assert(wait_until(30, 1, __check_if_status, dut, dut_port, 'up'), "dut port {} didn't go down as expected".format(dut_port))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks

@OleksandrKozodoi
Copy link
Copy Markdown
Contributor Author

OleksandrKozodoi commented Nov 2, 2020

@yxieca I've added your changes #2411 to my PR, but I don't have write access for resolving conflicts. Could you please review my changes and resolve conflicts. Thanks.

@OleksandrKozodoi Sorry I cannot solve the merge conflict either, it appears that the conflict is too complicated to be solved by web tool. It might be quicker if you create another change and force push to your fork? Sorry for the inconvenience.

@yxieca I've resolved conflicts. Could you please review my changes? Thanks.

OleksandrKozodoi and others added 13 commits November 2, 2020 16:57
Signed-off-by: Oleksandr Kozodoi <oleksandrx.kozodoi@intel.com>
Signed-off-by: Oleksandr Kozodoi <oleksandrx.kozodoi@intel.com>
Signed-off-by: Oleksandr Kozodoi <oleksandrx.kozodoi@intel.com>
Signed-off-by: Oleksandr Kozodoi <oleksandrx.kozodoi@intel.com>
Signed-off-by: Oleksandr Kozodoi <oleksandrx.kozodoi@intel.com>
Co-authored-by: Danny Allen <daall@microsoft.com>
Signed-off-by: Oleksandr Kozodoi <oleksandrx.kozodoi@intel.com>
Signed-off-by: Oleksandr Kozodoi <oleksandrx.kozodoi@intel.com>
Signed-off-by: Oleksandr Kozodoi <oleksandrx.kozodoi@intel.com>
Signed-off-by: Oleksandr Kozodoi <oleksandrx.kozodoi@intel.com>
Signed-off-by: Oleksandr Kozodoi <oleksandrx.kozodoi@intel.com>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 3, 2020

This pull request introduces 3 alerts when merging cfd0655 into 1306417 - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call
  • 1 for Unused import

@OleksandrKozodoi
Copy link
Copy Markdown
Contributor Author

@yxieca @wangxin @daall @bingwang-ms Please review

@NazarTkachuk
Copy link
Copy Markdown
Contributor

retest vsimage please

Copy link
Copy Markdown
Contributor

@daall daall left a comment

Choose a reason for hiding this comment

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

LGTM, though I think Ying's feedback would be good to incorporate

Signed-off-by: Oleksandr Kozodoi <oleksandrx.kozodoi@intel.com>
@yxieca yxieca merged commit 0687784 into sonic-net:master Nov 5, 2020
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.

7 participants