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

Tests: Add software tests and CI configuration #8

Merged
merged 2 commits into from
Apr 24, 2024
Merged

Tests: Add software tests and CI configuration #8

merged 2 commits into from
Apr 24, 2024

Conversation

amotl
Copy link
Contributor

@amotl amotl commented Apr 22, 2024

About

This patch adds a few test cases, in order to support subsequent refactorings.

Details

Contrary to previous patches, in this case, the main code base has been touched, but only at spots to improve log message formatting, and, on a single occasion, to improve exception handling and propagation.

The changes were needed to have reasonably sensible software tests without embedding too many quirks or code smells, and should also affect the implementation in a positive way, hopefully not breaking it.

References

@amotl amotl requested a review from peekjef72 April 22, 2024 22:27
setup.py Outdated
Comment on lines 50 to 52
"Programming Language :: Python :: 3.6",
"Programming Language :: Python :: 3.7",
"Programming Language :: Python :: 3.8",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you comfortable with removing support for those versions of Python? In fact, it is only about the software tests, which utilize importlib.resources.files, only available on Python>=3.9.

Bringing in a backward-compatibility package, in this case importlib-resources, would not be that hard, and just a few more lines of code away.

So, just let me know if you would prefer retaining support for those.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure...
Linux Red Hat 7 python base version is still 3.6, it means that it is sill used at least in my company. In the other hand, in my old use-case, dashboards came from sandbox hosts that are in rh8 or 9 with higher version of python...

do you think it is possible to keep them ? probably with a conditionnal import

PS: Can you imagine I'm currently working on a small tool that has to be tested on RH 6 with only python 2.7!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I will re-add support down to Python 3.6.

Copy link
Contributor Author

@amotl amotl Apr 23, 2024

Choose a reason for hiding this comment

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

a333ab6 re-establishes compatibility of the test suite with Python 3.6.

The main code base was compatible with Python 3.6 before, and did not undergo any changes in this regard. So, this patch is exclusively about adjusting the test suite.

],
"test": [
"grafana-dashboard==0.1.1",
"pydantic<2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pydantic dependency may have slipped in, and can probably be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No idea where that comes from...
Maybe I have duplicated a setup.py from an another tool without knowing what I do... (that is really possible!)
Feel free to remove it is not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it slipped in on my updates, and the PR comment was merely a note to self. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found out why it was needed: Pydantic 2.x croaks on Python 3.12, due to how it is used by grafana-dashboard.

Comment on lines 11 to 27
@pytest.fixture(scope="session", autouse=True)
def niquests_patch_all():
"""
Patch module namespace, pretend Niquests is Requests.
"""
from sys import modules

import niquests
import urllib3

# Amalgamate the module namespace to make all modules aiming
# to use `requests`, in fact use `niquests` instead.
modules["requests"] = niquests
modules["requests.adapters"] = niquests.adapters
modules["requests.sessions"] = niquests.sessions
modules["requests.exceptions"] = niquests.exceptions
modules["requests.packages.urllib3"] = urllib3
Copy link
Contributor Author

@amotl amotl Apr 22, 2024

Choose a reason for hiding this comment

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

It might only be a nit, but I would like to highlight such procedures better on behalf of the Niquests documentation, in order to make the road less bumpy for people writing software tests for applications or libraries which are based on Niquests.

Now, I only need to find the time. Linking it to the issue on their tracker is the best I can do for now.

/cc @Ousret

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should replace peekjef72 by grafana-toolbox

Copy link
Contributor Author

@amotl amotl Apr 24, 2024

Choose a reason for hiding this comment

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

Thanks for spotting. I've diverted this adjustment to GH-11.

Copy link
Collaborator

@peekjef72 peekjef72 left a comment

Choose a reason for hiding this comment

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

good job!
Ready to merge.

@amotl amotl merged commit 8c184f7 into main Apr 24, 2024
2 checks passed
@amotl amotl deleted the tests branch April 24, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants