-
Notifications
You must be signed in to change notification settings - Fork 80
Store distgit and namespace in source_json #4128
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
base: main
Are you sure you want to change the base?
Store distgit and namespace in source_json #4128
Conversation
frontend/coprs_frontend/tests/test_views/test_coprs_ns/test_method_distgit.py
Fixed
Show fixed
Hide fixed
frontend/coprs_frontend/tests/test_views/test_coprs_ns/test_method_distgit.py
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds distgit and namespace to the source_json for dist-git builds, which is a useful enhancement. The logic change in builds_logic.py is correct and well-implemented. The accompanying test changes are also good, with an update to an existing test and the addition of a new one to cover the new functionality. My review includes a couple of suggestions for the test file: removing an unused import to keep the code clean, and improving the assertions in the new test to make it more comprehensive and robust, ensuring all relevant fields in source_json are verified. Overall, this is a good contribution. Addressing the suggested changes will further improve the quality of the tests.
frontend/coprs_frontend/tests/test_views/test_coprs_ns/test_method_distgit.py
Outdated
Show resolved
Hide resolved
| source_dict = json.loads(build.source_json) | ||
| assert "namespace" in source_dict | ||
| assert "distgit" in source_dict | ||
| assert source_dict["namespace"] == "forks/testuser" | ||
| assert source_dict["distgit"] == "fedora" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertions in this test are not comprehensive. It's better to assert the entire source_json content to ensure all fields, including clone_url and committish, are correctly set. This makes the test more robust and consistent with test_copr_user_can_add_distgit_build.
| source_dict = json.loads(build.source_json) | |
| assert "namespace" in source_dict | |
| assert "distgit" in source_dict | |
| assert source_dict["namespace"] == "forks/testuser" | |
| assert source_dict["distgit"] == "fedora" | |
| expected_source_json = { | |
| "clone_url": "https://src.fedoraproject.org/forks/testuser/rpms/mock", | |
| "distgit": "fedora", | |
| "namespace": "forks/testuser", | |
| "committish": "master", | |
| } | |
| assert json.loads(build.source_json) == expected_source_json |
c28d107 to
357cb26
Compare
|
357cb26 to
0cbf968
Compare
Pull Request validationFailed🔴 Review - Missing review from a member (2 required) Success🟢 CI - All checks have passed |
Fixes #3609