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

pytest conversion for tests/foreman/cli/test_oscap.py #8011

Merged
merged 4 commits into from
Oct 23, 2020
Merged

pytest conversion for tests/foreman/cli/test_oscap.py #8011

merged 4 commits into from
Oct 23, 2020

Conversation

jameerpathan111
Copy link
Contributor

@jameerpathan111 jameerpathan111 commented Sep 25, 2020 •
edited
Loading

Test result:

$ pytest tests/foreman/cli/test_oscap.py 
============================= test session starts ==============================
platform linux -- Python 3.6.8, pytest-4.6.3, py-1.8.0, pluggy-0.13.1
shared_function enabled - OFF - scope:  - storage: file
rootdir: /home/jpathan/projects/robottelo
plugins: services-1.3.1, xdist-1.30.0, mock-1.10.4, cov-2.7.1, forked-1.1.3
2020-09-25 13:19:02 - conftest - DEBUG - Collected 31 test cases
collected 31 items

tests/foreman/cli/test_oscap.py ............................sss          [100%]

============== 28 passed, 3 skipped, 9 warnings in 893.58 seconds ==============

@jameerpathan111 jameerpathan111 self-assigned this Sep 25, 2020
@codecov
Copy link

codecov bot commented Sep 25, 2020 •
edited
Loading

Codecov Report

Merging #8011 into master will decrease coverage by 7.70%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8011      +/-   ##
==========================================
- Coverage   66.27%   58.57%   -7.71%     
==========================================
  Files          35       74      +39     
  Lines        4175     5793    +1618     
==========================================
+ Hits         2767     3393     +626     
- Misses       1408     2400     +992
Impacted Files Coverage Δ
robottelo/cli/factory.py 25.30% <0.00%> (ø)
robottelo/cli/architecture.py 100.00% <0.00%> (ø)
robottelo/cli/ldapauthsource.py 100.00% <0.00%> (ø)
robottelo/cli/ansible.py 60.00% <0.00%> (ø)
robottelo/cli/partitiontable.py 100.00% <0.00%> (ø)
robottelo/cli/location.py 52.38% <0.00%> (ø)
robottelo/cli/discoveryrule.py 100.00% <0.00%> (ø)
robottelo/cli/contentview.py 43.13% <0.00%> (ø)
robottelo/cli/lifecycleenvironment.py 69.23% <0.00%> (ø)
robottelo/cli/host.py 40.35% <0.00%> (ø)
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ff06d4...18b54b3. Read the comment docs.

@jameerpathan111
Copy link
Contributor Author

jameerpathan111 commented Oct 7, 2020 •
edited
Loading

@SatelliteQE/robottelo-tier-1-reviewers @SatelliteQE/robottelo-tier-2-reviewers need your review.

lhellebr
lhellebr approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@lhellebr lhellebr left a comment

Choose a reason for hiding this comment

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

Looks good! ACK

@jameerpathan111 jameerpathan111 added the Tier 1 ACK A Tier 1 reviewer has ACK'd this PR label Oct 7, 2020
mirekdlugosz
mirekdlugosz reviewed Oct 7, 2020
View reviewed changes
Copy link
Contributor

@mirekdlugosz mirekdlugosz left a comment

Choose a reason for hiding this comment

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

Overall looks OK, but I'm unsure about introducing CLI methods inside api_fixtures. I'll leave it to @jyejare to decide if that's ok and possibly merge PR.

pytest_fixtures/api_fixtures.py Outdated Show resolved Hide resolved
@jameerpathan111
Copy link
Contributor Author

Overall looks OK, but I'm unsure about introducing CLI methods inside api_fixtures. I'll leave it to @jyejare to decide if that's ok and possibly merge PR.

We don't have nailgun support for it so I used CLI instead. I have created SatelliteQE/nailgun#753 issue for it and will work on it soon.

@jameerpathan111
Copy link
Contributor Author

Updated test results:

$ pytest tests/foreman/cli/test_oscap.py | tee scap.log
============================= test session starts ==============================
platform linux -- Python 3.6.8, pytest-4.6.3, py-1.8.0, pluggy-0.13.1
shared_function enabled - OFF - scope:  - storage: file
rootdir: /home/jpathan/projects/robottelo
plugins: services-1.3.1, xdist-1.30.0, mock-1.10.4, cov-2.7.1, forked-1.1.3
2020-10-08 07:08:56 - conftest - DEBUG - Collected 31 test cases
collected 31 items

tests/foreman/cli/test_oscap.py ............................sss          [100%]


============= 28 passed, 3 skipped, 9 warnings in 1364.38 seconds ==============

@jyejare
Copy link
Member

jyejare commented Oct 8, 2020

Overall looks OK, but I'm unsure about introducing CLI methods inside api_fixtures. I'll leave it to @jyejare to decide if that's ok and possibly merge PR.

We don't have nailgun support for it so I used CLI instead. I have created SatelliteQE/nailgun#753 issue for it and will work on it soon.

As suggested in the meeting, you should be adding ur CLI based fixture in separate fixture module. Later it will be moved to appropriate module with no impact on ur tests.

latran
latran approved these changes Oct 8, 2020
View reviewed changes
Copy link
Contributor

@latran latran left a comment

Choose a reason for hiding this comment

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

LGTM. I do have one question for @jyejare. Is there going to be an effort in expanding the pytest_fixture directory if we're heading in this direction for CLI? I feel that one CLI pytest_fixture is good enough (like api) unless there's a good reason otherwise.

@jyejare
Copy link
Member

jyejare commented Oct 8, 2020

@latran I would take ur question as whether Satellite Endpoints or Satellite Components !

Ans: Even though we divide the fixtures satellite-endpoints wise, the fixture module for each endpoint will have a huge number of fixtures aka bulky.

So either we can choose to :

  1. Divide them per Component wise (that is API, CLI, UI fixtures for a component in single fixture module) for all components. e.g Product, VmWare, User etc
  2. Divide them per Sat-management/high-level component wise. e.g ProvisioningManagement, RBAC, ContentManagement etc.

With 2nd all the related components will stay together with no bulky / no light fixture module.

Still always open for thoughts from @SatelliteQE/airgun-tier-2-reviewers !

jyejare
jyejare requested changes Oct 9, 2020
View reviewed changes
Copy link
Member

@jyejare jyejare left a comment

Choose a reason for hiding this comment

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

ACK pending comments / questions.

pytest_fixtures/smartproxy_fixtures.py Outdated Show resolved Hide resolved
pytest_fixtures/user_fixtures.py Show resolved Hide resolved
pytest_fixtures/user_fixtures.py Outdated Show resolved Hide resolved
tests/foreman/cli/test_oscap.py Outdated Show resolved Hide resolved
tests/foreman/cli/test_oscap.py Show resolved Hide resolved
tests/foreman/cli/test_oscap.py Show resolved Hide resolved
tests/foreman/cli/test_oscap.py Show resolved Hide resolved
tests/foreman/cli/test_oscap.py Show resolved Hide resolved
tests/foreman/cli/test_oscap.py Show resolved Hide resolved
tests/foreman/cli/test_oscap.py Show resolved Hide resolved
@JacobCallahan
Copy link
Member

@jyejare have all of your comments been addressed?

jyejare
jyejare approved these changes Oct 23, 2020
View reviewed changes
Copy link
Member

@jyejare jyejare left a comment

Choose a reason for hiding this comment

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

ACK pending resolve conflicts.

@jameerpathan111
Copy link
Contributor Author

ACK pending resolve conflicts.

@jyejare done.

@jyejare jyejare merged commit d8131a0 into SatelliteQE:master Oct 23, 2020
@jameerpathan111
Copy link
Contributor Author

Thanks everyone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mirekdlugosz mirekdlugosz mirekdlugosz left review comments

@jyejare jyejare jyejare approved these changes

@lhellebr lhellebr lhellebr approved these changes

@latran latran latran approved these changes

Assignees

@jameerpathan111 jameerpathan111

Labels
Tier 1 ACK A Tier 1 reviewer has ACK'd this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jameerpathan111 @jyejare @JacobCallahan @latran @lhellebr @mirekdlugosz