Add Ruff pytest standards #12796

JacobCallahan · 2023-10-03T16:07:19Z

This is a big one! Most of the changes are automatic, but a number of these are manual. I've deselected rules relating to fixture naming, but we can have a conversation later about whether we want to adopt the underscore naming conventions for fixtures.

JacobCallahan · 2023-10-03T16:08:03Z

There are a few ruff checks that will fail in this initial version until I can contact component owners to decide how we want to handle the recommendations. The most complex of which are using request.addfinalizer.

sambible · 2023-10-03T16:14:08Z

335 changes
Need to make a story on our board to review this bad boy

JacobCallahan · 2023-10-04T12:10:23Z

@sambible please do!

jyejare

Round 1 Review !

pytest_fixtures/component/satellite_auth.py

tests/foreman/api/test_acs.py

tests/foreman/api/test_capsulecontent.py

tests/foreman/api/test_http_proxy.py

tests/foreman/api/test_rhcloud_inventory.py

pytest_fixtures/component/satellite_auth.py

sambible · 2023-10-05T13:31:28Z

tests/foreman/api/test_bookmarks.py

-@pytest.mark.tier1
-@pytest.mark.parametrize('public', (True, False))
+@pytest.mark.tier1()
+@pytest.mark.parametrize('public', [True, False])


This is interesting, any particular reason for the change here

https://docs.astral.sh/ruff/rules/pytest-parametrize-values-wrong-type

sambible · 2023-10-05T13:34:57Z

tests/foreman/api/test_classparameters.py

@@ -137,15 +137,15 @@ def test_negative_update_parameter_type(self, test_data, module_puppet):
            2. Error raised for invalid default value.
        """
        sc_param = module_puppet['sc_params'].pop()
+        sc_param.override = True
+        sc_param.parameter_type = test_data['sc_type']
+        sc_param.default_value = test_data['value']


I guess it's taking out the stuff from the raises block that's not a function call. idk how I feel about this stylistically

the point of this change is to make sure that only the call that raises the error should be in the context manager. there are no functional reasons why these extra lines should be under that context.

sambible · 2023-10-05T13:38:42Z

tests/foreman/api/test_media.py

    @pytest.mark.parametrize(
-        'name, new_name',
+        ('name', 'new_name'),


Does putting this in a tuple have any affect on the functionality here? I don't think so, but just curious

https://docs.astral.sh/ruff/rules/pytest-parametrize-names-wrong-type/

tests/foreman/api/test_repositories.py

sambible · 2023-10-05T13:44:24Z

tests/foreman/api/test_role.py

@@ -1238,6 +1238,7 @@ def test_positive_taxonomies_control_to_superadmin_without_org_admin(
        entities.User(id=user.id).delete()
        with pytest.raises(HTTPError):
            user_role.read()
+        with pytest.raises(HTTPError):


This looks worse to me than just having 2 calls in the same raises block

which is more functionally correct for the test, though?
shouldn't both raise HTTPErrors?
what if the user_role.read() doesn't raise an HTTPError but user.read() does?

You are definitely right, this is more correct. I'm assuming the second statement here hasn't ever executed, actually, since the first statement will always raise an error ( it's been deleted ) and you'd leave the ocntext

Griffin-Sullivan

Changes to Platform owned tests / fixtures look good to me!

ogajduse

This one is huge! ACK with minor and non-blocking comments.

pytest_fixtures/component/satellite_auth.py

tests/foreman/api/test_capsulecontent.py

tests/foreman/api/test_rhcloud_inventory.py

tests/foreman/api/test_http_proxy.py

tests/foreman/ui/test_discoveryrule.py

ogajduse · 2023-10-09T13:07:26Z

tests/foreman/virtwho/ui/test_nutanix.py

-        try:
+        with pytest.raises(Exception) as exc_info:  # noqa: PT011 - TODO determine better exception
            get_configure_option("ahv_internal_debug", config_file)
-        except Exception as VirtWhoError:
-            assert env_error == str(VirtWhoError)
+        assert str(exc_info.value) == env_error


@yanpliu Can you please have a look at this one? We do not have to address it in this PR necessarily, but it would be good to keep the existence of this TODO in mind.
https://docs.astral.sh/ruff/rules/pytest-raises-too-broad/

Note: this construct is present across the test modules that are owned by the virt-who team.

tests/foreman/longrun/test_provisioning_computeresource.py

adarshdubey-star

Changes to the rocket team components look valid to me.

shubhamsg199

Changes to Rocket team tests and fixtures look good to me.

Gauravtalreja1 · 2023-10-12T11:12:50Z

tests/upgrades/test_classparameter.py

+    @pytest.mark.usefixtures('_clean_scenario')
    @pytest.mark.parametrize('count', list(range(1, 10)))
    def test_post_puppet_class_parameter_data_and_type(
-        self, count, _clean_scenario, class_pre_upgrade_data, class_target_sat
+        self, count, class_pre_upgrade_data, class_target_sat


Just wondering, if this change to use usefixtures here made by ruff ?

this was a manual change based on one of the new rules ruff enforces.

JacobCallahan · 2023-10-20T17:28:42Z

Major update to the PR. After additional feedback, I've dropped the requirements for parentheses after fixtures and marks. I've also incorporated the logic changes previously discussed above.

jyejare

ACK pending minor comments !

tests/foreman/api/test_http_proxy.py

tests/foreman/ui/test_contenthost.py

jyejare · 2023-11-02T08:06:39Z

@JacobCallahan I was planning to merge it now ! And unfortunately I see conflicts holding it up ! Please resolve them :(

This is a big one! Most of the changes are automatic, but a number of these are manual. I've deselected rules relating to fixture naming, but we can have a conversation later about whether we want to adopt the underscore naming conventions for fixtures.

JacobCallahan added CherryPick PR needs CherryPick to previous branches 6.12.z Introduced in or relating directly to Satellite 6.12 6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 labels Oct 3, 2023

JacobCallahan requested review from a team as code owners October 3, 2023 16:07

JacobCallahan marked this pull request as draft October 3, 2023 16:07

github-actions bot assigned JacobCallahan Oct 3, 2023

jyejare requested changes Oct 4, 2023

View reviewed changes

sambible requested changes Oct 5, 2023

View reviewed changes

Griffin-Sullivan approved these changes Oct 5, 2023

View reviewed changes

ogajduse approved these changes Oct 9, 2023

View reviewed changes

adarshdubey-star reviewed Oct 10, 2023

View reviewed changes

tests/foreman/longrun/test_provisioning_computeresource.py Outdated Show resolved Hide resolved

adarshdubey-star approved these changes Oct 10, 2023

View reviewed changes

shubhamsg199 approved these changes Oct 12, 2023

View reviewed changes

Gauravtalreja1 reviewed Oct 12, 2023

View reviewed changes

JacobCallahan force-pushed the ruff_pyt branch 2 times, most recently from 15877b3 to b149a0b Compare October 20, 2023 17:26

JacobCallahan requested review from jyejare and sambible October 20, 2023 17:26

JacobCallahan force-pushed the ruff_pyt branch 2 times, most recently from 9aec969 to 0a67c98 Compare October 20, 2023 17:39

JacobCallahan marked this pull request as ready for review October 20, 2023 17:55

lpramuk approved these changes Oct 31, 2023

View reviewed changes

jyejare requested changes Oct 31, 2023

View reviewed changes

tests/foreman/api/test_http_proxy.py Show resolved Hide resolved

tests/foreman/ui/test_contenthost.py Outdated Show resolved Hide resolved

JacobCallahan force-pushed the ruff_pyt branch from 0a67c98 to 19ee4f5 Compare November 1, 2023 14:37

jyejare approved these changes Nov 2, 2023

View reviewed changes

Add Ruff pytest standards

fee22e8

This is a big one! Most of the changes are automatic, but a number of these are manual. I've deselected rules relating to fixture naming, but we can have a conversation later about whether we want to adopt the underscore naming conventions for fixtures.

JacobCallahan force-pushed the ruff_pyt branch from 19ee4f5 to fee22e8 Compare November 2, 2023 13:58

jyejare enabled auto-merge (squash) November 2, 2023 13:59

jyejare merged commit 83ca284 into SatelliteQE:master Nov 2, 2023
5 checks passed

This was referenced Nov 2, 2023

[Failed-AutoCherryPick] - Add Ruff pytest standards #13037

Closed

[Failed-AutoCherryPick] - Add Ruff pytest standards #13038

Closed

[Failed-AutoCherryPick] - Add Ruff pytest standards #13039

Closed

JacobCallahan deleted the ruff_pyt branch November 2, 2023 14:20

ogajduse mentioned this pull request Nov 21, 2023

Yield details always, no need to check for SCA eligibility #13141

Merged

This was referenced Nov 21, 2023

[6.14.z] Yield details always, no need to check for SCA eligibility #13143

Merged

[6.13.z] Yield details always, no need to check for SCA eligibility #13144

Merged

Gauravtalreja1 mentioned this pull request Dec 6, 2023

Fix subnet tests #13284

Merged

This was referenced Dec 6, 2023

[6.15.z] Fix subnet tests #13292

Merged

[6.14.z] Fix subnet tests #13293

Merged

[6.12.z] Fix subnet tests #13294

Merged

[6.13.z] Fix subnet tests #13295

Merged

Gauravtalreja1 mentioned this pull request Dec 7, 2023

Update error message in negative settings test #13307

Merged

pondrejk mentioned this pull request Dec 13, 2023

users and roles test fixes #13433

Merged

This was referenced Dec 13, 2023

[6.14.z] users and roles test fixes #13437

Merged

[6.15.z] users and roles test fixes #13439

Merged

JacobCallahan mentioned this pull request Jan 9, 2024

[6.12.z] Add Ruff rule group B (flake8-bugbear) #13697

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Ruff pytest standards #12796

Add Ruff pytest standards #12796

JacobCallahan commented Oct 3, 2023

JacobCallahan commented Oct 3, 2023 •

edited

Loading

sambible commented Oct 3, 2023

JacobCallahan commented Oct 4, 2023

jyejare left a comment

sambible Oct 5, 2023

JacobCallahan Oct 5, 2023

sambible Oct 5, 2023

JacobCallahan Oct 5, 2023

sambible Oct 5, 2023

JacobCallahan Oct 5, 2023 •

edited

Loading

sambible Oct 5, 2023

JacobCallahan Oct 5, 2023

sambible Oct 5, 2023

Griffin-Sullivan left a comment

ogajduse left a comment

ogajduse Oct 9, 2023

adarshdubey-star left a comment

shubhamsg199 left a comment

Gauravtalreja1 Oct 12, 2023

JacobCallahan Oct 20, 2023

JacobCallahan commented Oct 20, 2023

jyejare left a comment

jyejare commented Nov 2, 2023

Add Ruff pytest standards #12796

Add Ruff pytest standards #12796

Conversation

JacobCallahan commented Oct 3, 2023

JacobCallahan commented Oct 3, 2023 • edited Loading

sambible commented Oct 3, 2023

JacobCallahan commented Oct 4, 2023

jyejare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobCallahan Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Griffin-Sullivan left a comment

Choose a reason for hiding this comment

ogajduse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adarshdubey-star left a comment

Choose a reason for hiding this comment

shubhamsg199 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobCallahan commented Oct 20, 2023

jyejare left a comment

Choose a reason for hiding this comment

jyejare commented Nov 2, 2023

JacobCallahan commented Oct 3, 2023 •

edited

Loading

JacobCallahan Oct 5, 2023 •

edited

Loading