chore: add test verifying we only use specific Qt modules#998
chore: add test verifying we only use specific Qt modules#998crowecawcaw wants to merge 6 commits intoaws-deadline:mainlinefrom
Conversation
Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
github-actions
bot
added
the
waiting-on-maintainers
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import pytest | ||
|
|
||
| try: | ||
| from deadline.client.ui.dialogs import deadline_config_dialog # noqa: F401 |
Check notice
Code scanning / CodeQL
Unused import Note test
There was a problem hiding this comment.
Can you expand on why we are importing this? The noqa comment makes me think this is intentional but it isn't clear why we would need this for this test.
Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
|
|
||
| def test_config_dialog_imports_only_allowed_qt_modules(self): | ||
| """Verify deadline_config_dialog module only imports QtCore, QtGui, QtWidgets.""" | ||
| imported = _get_imported_qt_modules("dialogs.deadline_config_dialog") |
There was a problem hiding this comment.
Instead of manually adding the paths here, can we list the contents of the UI directory and include every file within it? Then we don't have to remember to add this test for additional modules.
crowecawcaw
removed
the
waiting-on-maintainers
Instead of manually listing each module path in individual test methods, dynamically discover all Python files under deadline.client.ui and parametrize the test. This way new modules are automatically covered without needing to update the test. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
|
| _TEST_SCRIPT = """ | ||
| import sys | ||
| module_to_import = sys.argv[1] | ||
| exec(f"import {module_to_import}") |
There was a problem hiding this comment.
exe() on sys.argv doesn't look like a good pattern, can we do something like this:
importlib.import_module(module_to_import)| except ImportError: | ||
| pytest.importorskip("deadline.client.ui.dialogs.deadline_config_dialog") | ||
|
|
||
| ALLOWED_QT_MODULES = {"QtCore", "QtGui", "QtWidgets", "QtOpenGL", "QtOpenGLWidgets"} |
There was a problem hiding this comment.
This is different than what line 4 claims.
| return set(output.split(",")) if output else set() | ||
|
|
||
|
|
||
| _ui_modules = _get_ui_module_paths() |
There was a problem hiding this comment.
Maybe move this inside a fixture or use pytest_generate_tests?
| result = subprocess.run( | ||
| [sys.executable, "-c", _TEST_SCRIPT, fully_qualified_module], | ||
| capture_output=True, | ||
| text=True, | ||
| ) |
There was a problem hiding this comment.
need a timeout in case it hangs
| modules = [] | ||
| for py_file in sorted(ui_dir.rglob("*.py")): | ||
| if py_file.name.startswith("_"): | ||
| continue |
There was a problem hiding this comment.
Can we document why this is skipped?
4 participants
What was the problem/requirement? (What/Why)
We're considering bundling Qt with some distributions of this software. Qt uses many licenses across its modules. We need to know which modules we're using and be alerted if we're every adding another one.
What was the solution? (How)
Add a unit test.
What is the impact of this change?
More confidence in the Qt modules we're depending on
How was this change tested?
Removed a module from the allow list and verified the test failed.
Was this change documented?
n/a
Does this PR introduce new dependencies?
No
Is this a breaking change?
No
Does this change impact security?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.