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

Refactor: Pytest refactoring and fixes for system test cases #414

Merged
merged 1 commit into from
Nov 23, 2021

Conversation

KonikaChaurasiya-GSLab
Copy link
Contributor

@KonikaChaurasiya-GSLab KonikaChaurasiya-GSLab commented Nov 11, 2021

Change Summary

Pytest refactoring

Related Issue(s)

Fixes #N/A

Component(s) name

Pytest

Proposed changes

Define a clear structure to separate:

  • Unit test with no CV interaction to test component: tests/unit
  • System test to run module backend component and which requires CV connection: tests/system
  • lib to provides a safe place for fixtures, parametrizes, mook data: tests/lib

How to test

# Go to test folder
$ cd tests/

# Run Pytest
$ pytest --html=report.html --self-contained-html --cov=. --cov-report=html --capture=tee-sys --color yes system

Checklist

User Checklist

  • N/A

Repository Checklist

  • My code has been rebased from devel before I start
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation and documentation have been updated accordingly. (check the box if not applicable)
  • I have updated molecule CI testing accordingly. (check the box if not applicable)

@github-actions github-actions bot added module: cv_container_v3 Issue related to cv_container module in v3 module: cv_device_v3 Issue related to cv_device module in v3 module_utils: cv_client cv_client implementation issue labels Nov 11, 2021
@KonikaChaurasiya-GSLab KonikaChaurasiya-GSLab marked this pull request as draft November 11, 2021 13:51
@KonikaChaurasiya-GSLab KonikaChaurasiya-GSLab changed the title Pytest refactoring and fixes for system test cases Refactor: Pytest refactoring and fixes for system test cases Nov 11, 2021
Copy link
Contributor

@guillaumeVilar guillaumeVilar left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR! Please find below my comments.

  1. The Makefile config.py rule did not get updated (still create the config.py for the on-prem CVP).
    We should update it to move away from on-prem IMHO - i.e. remove username, password and just have 2 fields:
    server and token.

  2. The git pre-commit validation failed because an extra line has been added at the end of the file tests/system/test_cv_device_tools_fqdn.py

  3. system/test_cv_container_tools.py > OK

  4. system/test_cv_device_tools_fqdn.py > OK

  5. system/test_cv_device_tools_generic.py > OK

  6. system/test_cv_device_tools_serial.py > OK

  7. system/test_cv_device_tools.py > OK but 2 skip.
    As mentionned, we can move one of the device to the undefinied container before running those tests by removing them from the inventory (CVaaS now automatically add the devices to the provisioning page).
    Sample example:

>>> from lib import config
>>> from cvprac.cvp_client import CvpClient
>>> cvp_client.connect(
...     nodes=[config.server],
...     username=config.username,
...     password=config.password,
...     is_cvaas=True,
...     api_token=config.user_token
... )
>>> cvp_client.api.delete_device('50:00:00:cb:38:c2')
{'result': 'success'}
  1. As a future improvement (maybe in a future PR), we could also add tests for device search by:
  • hostname
  • serial number
    This is not yet covered by the test suite as of now.

@KonikaChaurasiya-GSLab
Copy link
Contributor Author

@guillaumeVilar : Thanks for the code review. I have updated the PR as per your suggestions.
For Comment No. 8 As a future improvement : We already have the tests cases for device search_by hostname and serial number. I have set the self.inventory.search_by = FIELD_HOSTNAME and self.inventory.search_by = FIELD_SERIAL in the respective tests.

@KonikaChaurasiya-GSLab KonikaChaurasiya-GSLab marked this pull request as ready for review November 12, 2021 10:25
@titom73 titom73 added this to the v3.2.0 milestone Nov 17, 2021
Copy link
Contributor

@titom73 titom73 left a comment

Choose a reason for hiding this comment

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

See comments

tests/system/test_cv_device_tools.py Outdated Show resolved Hide resolved
Copy link
Contributor

@guillaumeVilar guillaumeVilar left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR. System tests now all passed, awesome!

A small detail, the Makefile was not updated with the changes:

AUTH_CONFIG_FILE = lib/config.py

define AUTH_CONFIG
#!/usr/bin/python
# coding: utf-8 -*-

username = "< username >"
password = "< password >"
server = "< cloudvision ip >"
endef
export AUTH_CONFIG

Moreover, I tried the other rules and are not triggering the system tests:

  • make ci > only run the unit tests
  • make test > same
  • make test-api > Give error:
===================================================================================================== no tests ran in 0.88s ======================================================================================================
ERROR: Wrong expression passed to '-m': generic,api: at column 8: unexpected character ","

make: *** [Makefile:32: test-api] Error 4

Copy link
Contributor

@guillaumeVilar guillaumeVilar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@titom73 titom73 left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome work!

@titom73 titom73 merged commit 5456fdd into aristanetworks:devel Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: cv_container_v3 Issue related to cv_container module in v3 module: cv_device_v3 Issue related to cv_device module in v3 module_utils: cv_client cv_client implementation issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants