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

fix in hosts param crud test #15596

Merged
merged 1 commit into from
Jul 11, 2024
Merged

Conversation

pondrejk
Copy link
Contributor

@pondrejk pondrejk commented Jul 9, 2024

Problem Statement

assertion error

Solution

reacting to change in param object

@pondrejk pondrejk added TestFailure Issues and PRs related to a test failing in automation No-CherryPick PR doesnt need CherryPick to previous branches labels Jul 9, 2024
@pondrejk pondrejk requested review from a team July 9, 2024 08:44
@pondrejk pondrejk self-assigned this Jul 9, 2024
@pondrejk pondrejk requested a review from a team as a code owner July 9, 2024 08:44
@pondrejk
Copy link
Contributor Author

pondrejk commented Jul 9, 2024

trigger: test-robottelo
pytest: tests/foreman/cli/test_host.py::test_positive_parameter_crud

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7672
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_host.py::test_positive_parameter_crud --external-logging
Test Result : ================== 1 passed, 9 warnings in 668.81s (0:11:08) ===================

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Jul 9, 2024
Copy link
Contributor

@vijaysawant vijaysawant left a comment

Choose a reason for hiding this comment

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

Why we are changing the older way of assertion? what is return value of host['parameters'] ?
I would assert with exact name & value rather asserting with list type object.

assert name == host['parameters']['name']
assert value == host['parameters']['value']

Comment on lines 1108 to 1109
assert name in [param["name"] for param in host['parameters']]
assert value in [param["value"] for param in host['parameters']]
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to continue with these new changes then I would suggest to use single quote for string to maintain consistant pattern

Suggested change
assert name in [param["name"] for param in host['parameters']]
assert value in [param["value"] for param in host['parameters']]
assert name in [param['name'] for param in host['parameters']]
assert value in [param['value'] for param in host['parameters']]

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we merge these assertions like "(name,value) in [(param['name'], param['value']) for param in host['parameters']]"

Comment on lines 1115 to 1116
assert name in [param["name"] for param in host['parameters']]
assert new_value in [param["value"] for param in host['parameters']]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert name in [param["name"] for param in host['parameters']]
assert new_value in [param["value"] for param in host['parameters']]
assert name in [param['name'] for param in host['parameters']]
assert new_value in [param['value'] for param in host['parameters']]


target_sat.cli.Host.delete_parameter({'host-id': host['id'], 'name': name})
host = target_sat.cli.Host.info({'id': host['id']})
assert name not in host['parameters']
assert name not in [param["name"] for param in host['parameters']]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert name not in [param["name"] for param in host['parameters']]
assert name not in [param['name'] for param in host['parameters']]

Comment on lines 1108 to 1109
assert name in [param["name"] for param in host['parameters']]
assert value in [param["value"] for param in host['parameters']]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we merge these assertions like "(name,value) in [(param['name'], param['value']) for param in host['parameters']]"

@pondrejk
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/cli/test_host.py::test_positive_parameter_crud

@Satellite-QE Satellite-QE removed the PRT-Passed Indicates that latest PRT run is passed for the PR label Jul 10, 2024
@pondrejk
Copy link
Contributor Author

Why we are changing the older way of assertion? what is return value of host['parameters'] ? I would assert with exact name & value rather asserting with list type object.

assert name == host['parameters']['name']
assert value == host['parameters']['value']
`, 

host['parameters'] is a list of dictionaries, something like [{name: 'a', value:'b'}]

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7692
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/cli/test_host.py::test_positive_parameter_crud --external-logging
Test Result : ================== 1 passed, 9 warnings in 645.04s (0:10:45) ===================

@Satellite-QE Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Jul 10, 2024
@vijaysawant
Copy link
Contributor

vijaysawant commented Jul 10, 2024

Why we are changing the older way of assertion? what is return value of host['parameters'] ? I would assert with exact name & value rather asserting with list type object.

assert name == host['parameters']['name']
assert value == host['parameters']['value']
`, 

host['parameters'] is a list of dictionaries, something like [{name: 'a', value:'b'}]

we can do something like this, can you please check and update?

for key, value in host['parameters'].items():
    assert (name, value) == (key, value)

This can be done in other better way

Copy link
Contributor

@vijaysawant vijaysawant left a comment

Choose a reason for hiding this comment

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

Thanks for work @pondrejk
Ack

Copy link
Contributor

@shweta83 shweta83 left a comment

Choose a reason for hiding this comment

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

Ack.

@pondrejk
Copy link
Contributor Author

we can do something like this, can you please check and update?

for key, value in host['parameters'].items():
    assert (name, value) == (key, value)

This can be done in other better way

I think that would only work with a single parameter assigned to a host, since we use function host I suppose there could be more params

@pondrejk pondrejk merged commit 1eee482 into SatelliteQE:master Jul 11, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No-CherryPick PR doesnt need CherryPick to previous branches PRT-Passed Indicates that latest PRT run is passed for the PR TestFailure Issues and PRs related to a test failing in automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants