Modify Manifester class to work with MockStub #4

synkd · 2022-06-01T21:26:57Z

Add a class to test_manifester.py to enable requests to be sent to a
MockStub version of the requests package instead of sending live
requests to the API. Modify the Manifester class to use the
MockStub-based requests when running unit tests. Add a helper function
to generate mock HTTP response codes.

This PR should start as a draft, as there is currently an issue with
integrating simple_retry with the RhsmApiStub class:

http://pastebin.test.redhat.com/1055597

Add a class to test_manifester.py to enable requests to be sent to a MockStub version of the requests package instead of sending live requests to the API. Modify the Manifester class to use the MockStub-based requests when running unit tests. Add a helper function to generate mock HTTP response codes.

This commit makes several modifications to the test harness, helper functions, and manifester itself to get the first two unit tests into a working state.

This commit adds a test for manifester's get_manifest() method, which is used to generate a manifest and write it locally. It also adds docstrings to functions in helpers.py and test_manifester.py that did not have docstrings previously.

This commit adds a test for Manifester's `delete_subscription_allocation` method and a `delete` method to the `RhsmApiStub` class to handle HTTP DELETE requests.

This commit adds a negative test for the timeout on exporting a manifest. It also modifies the return values of the mocked HTTP verb methods to reuse the same RhsmApiStub object throughout each execution, rather than returning a new RhsmApiStub object with each method call. It also modifies the manifester class and helper methods to support this change.

This commit adds a test of manifester's ability to handle the RHSM API's result pagination when attempting to retrieve all the subscription pools in an account containing more than 50 subscription pools. It also modifies the Manifester and RhsmApiStub classes to support the test.

This commit brings the mock subscription pool response data more in line with what is returned by the RHSM API in order to simplify some of the conditional logic that the Manifester class uses to process real vs mocked data.

This commit adds a test to ensure that each of the subscriptions in manifester_settings.yaml was successfully added to the subscription allocation created during a `get_manifest()` execution.

This commit adds a test to ensure that Manifester will replace an invalid sat_version value with the most recent valid sat_version. Since this test reuses the same manifest_data that was previously defined in another test, it also moves that manifest_data to a global variable.

This commit adds reworks the pre-commit hooks used by manifester. It includes changes to several files due to reformatting and other changes from black and ruff.

JacobCallahan

ACK on most everything. Just one request for your Ruff rules. This will require you to re-run ruff against the project.

pyproject.toml

This commit switches from using black for formatting and ruff for linting to using ruff for both of these. It also implements a more complete pyproject.toml and includes many small changes to get the project into a passing state for ruff. Additionally, it removes setup.py and setup.cfg as these are now redundant with pyproject.toml.

github-advanced-security · 2023-12-04T16:56:53Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

JacobCallahan

Couple of small things

JacobCallahan · 2023-12-04T19:39:50Z

pyproject.toml

+    "Natural Language :: English",
+    "Programming Language :: Python :: 3",
+    "Programming Language :: Python :: 3.10",
+    "Programming Language :: Python :: 3.11",


Since you have it in your checks, you can ad py 3.12 to this list.

JacobCallahan · 2023-12-04T19:41:51Z

tests/test_manifester.py

@@ -11,7 +11,7 @@
 manifest_data = {
    "log_level": "debug",
    "offline_token": "test",
-    "proxies": {"https:" ""},
+    "proxies": {"https:"},


is this meant to be a set or a dictionary?

It's meant to be a dictionary. Good catch, I'll correct that.

Since the manifester repo does not contain a fully populated config file, this commit switches all of the tests to use the manifest_data dictionary defined in the test_manifester.py module as the value of manifest_category when instantiating a Manifester object. It also add Python 3.12 to the list of classifiers in pyproject.toml.

synkd · 2023-12-05T19:41:00Z

Looking at the CodeQL failures, the checkout action is merging in commit 79a4b4e [1], although the most recent commit in this PR is 2af7760. I'm trying to determine why this is happening, and I'm open to suggestions if anyone has see something like this before.

[1] https://github.com/SatelliteQE/manifester/actions/runs/7091796806/job/19342080413#step:2:77

JacobCallahan · 2023-12-05T20:29:44Z

Strange, I've not seeing that before. Are the tests passing locally?
Perhaps doing an interactive rebase and squashing some commits might help. shot in the dark..

synkd · 2023-12-05T20:47:19Z

@JacobCallahan The change in 2bc2572 seems to have it now pulling the proper commit, although the tests are still failing in a way that they aren't locally.

The tests are passing locally generally, but I'm finding there's more flakiness than I'm comfortable with when I run all the tests in the module together. I think this is a result of me making the status_code method of the RhsmApiStub class a cached_property. It's getting stuck on a bad status code sometimes, and I think the answer is to delete the status code in more places than I'm doing currently. I'm going to work on getting that tightened up, and then I'll come back to the test failures in the CodeQL job.

Griffin-Sullivan

Haven't gotten to tests yet. Mostly just being picky about constants. There's a couple other places you used these. Definitely non-blocking just stylistic.

.github/workflows/codeql-analysis.yml

manifester/helpers.py

manifester/manifester.py

synkd · 2023-12-12T20:02:56Z

I'm not sure if I have fully resolved the flakiness issue, but I just had three successful local runs of all the tests, so I'm moving on for now. Here's one, just for the record:

$ pytest tests/test_manifester.py
============================================================================================================================= test session starts =============================================================================================================================
platform linux -- Python 3.11.6, pytest-7.4.3, pluggy-1.3.0
rootdir: /home/dsynk/manifester
configfile: pyproject.toml
collected 10 items                                                                                                                                                                                                                                                            

tests/test_manifester.py ..........                                                                                                                                                                                                                                     [100%]

======================================================================================================================= 10 passed in 132.35s (0:02:12) ========================================================================================================================

I was able to reproduce the CI failure by renaming my local manifester_settings.yaml, so something about processing the manifest data as a dict is not functioning properly. I'll work on debugging that next.

synkd · 2023-12-12T21:14:54Z

Ok, the issue with tests failing was due to some get() calls defaulting to pulling values from settings in a few places. I'm actually having trouble understanding why exactly this is happening. My best guess is that the way that Robottelo initializes settings causes the settings object to have a URL attribute and that this is not the case for the way that Manifester initializes settings. With the get() defaults removed, the tests pass, and Robottelo tests that use Manifester are still passing as well. I don't think this change will affect any current implementations of Manifester, but I'm open to arguments on whether I should find a different workaround.

To address the test flakiness issue, I've also changed the default fail_rate for HTTP response codes to 0 for most tests. I'm open to arguments on this point as well, but I don't think there is a major advantage to randomly exercising simple_retry. I would be willing to write a separate test with a less deterministic fail rate and dig into this issue further, but I would prefer to do that in a separate PR at this point.

This commit removes default values in several `get()` calls that were pulling from settings and causing the unit tests to fail in CI. It also changes the default `fail_rate` for HTTP response codes to 0 to address an issue in which a bad response code would loop permanently, causing unpredictable test flakiness.

synkd added the DRAFT Work in progress label Jun 1, 2022

synkd force-pushed the add_testing_capabilities branch from 4973169 to 5dcf41c Compare November 8, 2023 19:40

Rebase PR and address merge conflicts

0996b26

synkd force-pushed the add_testing_capabilities branch from 5dcf41c to 0996b26 Compare November 8, 2023 19:41

Get initial tests passing

36ba344

This commit makes several modifications to the test harness, helper functions, and manifester itself to get the first two unit tests into a working state.

synkd self-assigned this Nov 15, 2023

synkd added 16 commits November 15, 2023 15:07

Add test for get_manifest(), add docstrings

ac11f42

This commit adds a test for manifester's get_manifest() method, which is used to generate a manifest and write it locally. It also adds docstrings to functions in helpers.py and test_manifester.py that did not have docstrings previously.

Add test for delete_subscription_allocation

c4880fa

This commit adds a test for Manifester's `delete_subscription_allocation` method and a `delete` method to the `RhsmApiStub` class to handle HTTP DELETE requests.

Add negative test for simple retry's timeout

d7c4861

Remove extraneous comments

809bd13

Revert max_timeout change

3202681

Correct logic error in sat version evaluation

1149cb7

Use more realistic data in version check

f992b47

Fix failing test for simple retry timeout

6a43c1c

Add test for reading manifest data from dictionary

f2a897e

Use realistic data to simplify mock handling

7e45b04

This commit brings the mock subscription pool response data more in line with what is returned by the RHSM API in order to simplify some of the conditional logic that the Manifester class uses to process real vs mocked data.

Add test for correct subs in allocation

4fe55e0

This commit adds a test to ensure that each of the subscriptions in manifester_settings.yaml was successfully added to the subscription allocation created during a `get_manifest()` execution.

Tweak test_correct_subs_added_to_allocation

341cada

Add pre-commit hooks and run pre-commit

f53bfaa

This commit adds reworks the pre-commit hooks used by manifester. It includes changes to several files due to reformatting and other changes from black and ruff.

synkd removed the DRAFT Work in progress label Nov 30, 2023

synkd requested review from JacobCallahan and Griffin-Sullivan November 30, 2023 21:00

JacobCallahan requested changes Dec 1, 2023

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

synkd requested a review from JacobCallahan December 4, 2023 16:55

JacobCallahan requested changes Dec 4, 2023

View reviewed changes

synkd added 3 commits December 4, 2023 14:46

Modify workflow to workaround CodeQL failure

6e1a3d2

Add pytest dep and correct typo in test manifest_data

79a4b4e

Modify checkout action to pull latest commit from PR

2bc2572

Griffin-Sullivan approved these changes Dec 7, 2023

View reviewed changes

.github/workflows/codeql-analysis.yml Show resolved Hide resolved

manifester/helpers.py Show resolved Hide resolved

manifester/manifester.py Show resolved Hide resolved

manifester/manifester.py Outdated Show resolved Hide resolved

manifester/manifester.py Outdated Show resolved Hide resolved

Address reviewer feedback; delete status code in one additional spot

39483fa

synkd requested review from Griffin-Sullivan and JacobCallahan December 12, 2023 21:28

JacobCallahan approved these changes Dec 14, 2023

View reviewed changes

synkd merged commit 85c2a4c into SatelliteQE:master Dec 15, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify Manifester class to work with MockStub #4

Modify Manifester class to work with MockStub #4

synkd commented Jun 1, 2022

JacobCallahan left a comment

github-advanced-security bot commented Dec 4, 2023

JacobCallahan left a comment

JacobCallahan Dec 4, 2023

JacobCallahan Dec 4, 2023

synkd Dec 4, 2023

synkd commented Dec 5, 2023

JacobCallahan commented Dec 5, 2023

synkd commented Dec 5, 2023

Griffin-Sullivan left a comment

synkd commented Dec 12, 2023

synkd commented Dec 12, 2023

Modify Manifester class to work with MockStub #4

Modify Manifester class to work with MockStub #4

Conversation

synkd commented Jun 1, 2022

JacobCallahan left a comment

Choose a reason for hiding this comment

github-advanced-security bot commented Dec 4, 2023

JacobCallahan left a comment

Choose a reason for hiding this comment

JacobCallahan Dec 4, 2023

Choose a reason for hiding this comment

JacobCallahan Dec 4, 2023

Choose a reason for hiding this comment

synkd Dec 4, 2023

Choose a reason for hiding this comment

synkd commented Dec 5, 2023

JacobCallahan commented Dec 5, 2023

synkd commented Dec 5, 2023

Griffin-Sullivan left a comment

Choose a reason for hiding this comment

synkd commented Dec 12, 2023

synkd commented Dec 12, 2023