Add CI tests and fix all tests#1
Merged
aesteve-rh merged 9 commits intomainfrom Sep 11, 2025
Merged
Conversation
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Reviewer's GuideThis PR introduces a comprehensive GitHub Actions CI pipeline, streamlines the developer README, enhances the CLI with a --version flag, updates tox testing paths, and adjusts the test suites to match new logging behavior and schema conventions. Sequence diagram for CI pipeline job executionsequenceDiagram
participant Dev as Developer
participant GitHub as GitHub Actions
participant Lint as Linting Job
participant Format as Formatting Job
participant Type as Type Checking Job
participant Test as Test Job
Dev->>GitHub: Push or PR to main
GitHub->>Lint: Start linting
GitHub->>Format: Start formatting
GitHub->>Type: Start type checking
Lint-->>GitHub: Linting complete
Format-->>GitHub: Formatting check complete
Type-->>GitHub: Type checking complete
GitHub->>Test: Start tests (after all pre-stage jobs)
Test-->>GitHub: Test results
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- You’ve removed the detailed project structure and architecture section from README.developers.md — consider moving essential parts into a separate doc or brief overview so new contributors still have a high-level orientation.
- The CI coverage commands in both tox.ini and the GitHub workflow target
git_crossrefbut the code lives undersrc/git_crossref; please verify the coverage paths match your source layout to avoid gaps. - The formatting job only checks
src/but not thetests/directory — includingtests/in your black/isort checks will keep formatting consistent across the entire repo.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’ve removed the detailed project structure and architecture section from README.developers.md — consider moving essential parts into a separate doc or brief overview so new contributors still have a high-level orientation.
- The CI coverage commands in both tox.ini and the GitHub workflow target `git_crossref` but the code lives under `src/git_crossref`; please verify the coverage paths match your source layout to avoid gaps.
- The formatting job only checks `src/` but not the `tests/` directory — including `tests/` in your black/isort checks will keep formatting consistent across the entire repo.
## Individual Comments
### Comment 1
<location> `tests/test_main.py:213` </location>
<code_context>
+ result = runner.invoke(cli, ["init"], input="y\n")
+ assert result.exit_code == 0
+ mock_logger.warning.assert_called_with("Configuration file already exists: %s", config_path)
+ # When file exists, no output to stdout, just warning to stderr
+ assert result.output == ""
@patch("git_crossref.main.get_config_path")
</code_context>
<issue_to_address>
Consider testing the case where the configuration file exists but is not writable.
Adding a test for a non-writable configuration file will help verify that permission errors are handled correctly by the CLI.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
with patch("git_crossref.main.logger") as mock_logger:
result = runner.invoke(cli, ["init"], input="y\n")
assert result.exit_code == 0
mock_logger.warning.assert_called_with("Configuration file already exists: %s", config_path)
=======
with patch("git_crossref.main.logger") as mock_logger:
result = runner.invoke(cli, ["init"], input="y\n")
assert result.exit_code == 0
mock_logger.warning.assert_called_with("Configuration file already exists: %s", config_path)
def test_init_file_exists_not_writable(self, mock_get_config_path, runner, temp_dir):
import os
config_path.write_text("existing config")
# Make the file read-only
config_path.chmod(0o444)
mock_get_config_path.return_value = config_path
with patch("git_crossref.main.logger") as mock_logger:
result = runner.invoke(cli, ["init"], input="y\n")
# Should fail due to permission error
assert result.exit_code != 0
mock_logger.error.assert_called()
# Optionally, check the error message contains permission denied
error_args = mock_logger.error.call_args[0]
assert "Permission denied" in error_args[0] or "permission" in error_args[0].lower()
# Restore permissions so temp cleanup works
config_path.chmod(0o666)
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `tests/test_main.py:296` </location>
<code_context>
+ with patch("git_crossref.main.logger") as mock_logger:
+ result = runner.invoke(cli, ["clone", "--remote", "nonexistent"])
+ assert result.exit_code == 1
+ mock_logger.error.assert_called_with("Remote '%s' not found in configuration", "nonexistent")
class TestCleanCommand:
</code_context>
<issue_to_address>
Test does not verify the CLI output for user-facing error messages.
Please add an assertion to check that the CLI displays a clear error message to the user.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
result = runner.invoke(cli, ["clone", "--remote", "nonexistent"])
assert result.exit_code == 1
mock_logger.error.assert_called_with("Remote '%s' not found in configuration", "nonexistent")
=======
result = runner.invoke(cli, ["clone", "--remote", "nonexistent"])
assert result.exit_code == 1
mock_logger.error.assert_called_with("Remote '%s' not found in configuration", "nonexistent")
assert "Remote 'nonexistent' not found in configuration" in result.output
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `tests/test_schema.py:59` </location>
<code_context>
- assert path == schema_file
-
- def test_get_schema_path_not_exists(self, temp_dir):
+ # Test that get_schema_path returns a valid path or None
+ path = get_schema_path()
+ # In development environment, schema should exist
+ # In tox environment, it might not be copied, so allow None
+ if path is not None:
+ assert path.name == "gitcrossref-schema.json"
+ assert path.exists()
+ # Test passes if path is None (schema not found) or points to existing file
+
+ def test_get_schema_path_not_exists(self):
</code_context>
<issue_to_address>
Test for get_schema_path is environment-dependent and may be flaky.
Parametrize the test or use controlled mocking to ensure consistent results across environments and prevent CI flakiness.
</issue_to_address>
### Comment 4
<location> `README.developers.md:161` </location>
<code_context>
-index-servers =
- pypi
- testpypi
+4. Upload to the package to pypi:
-[pypi]
</code_context>
<issue_to_address>
Typo: 'Upload to the package to pypi' should be 'Upload the package to PyPI'.
Please update the phrase to 'Upload the package to PyPI' for accuracy.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
8ddb70f to
77b18e4
Compare
Signed-off-by: Albert Esteve <aesteve@redhat.com>
77b18e4 to
537b839
Compare
Signed-off-by: Albert Esteve <aesteve@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add CI pipeline, introduce version flag, streamline developer documentation, update tox config, and fix tests for new logging and schema behaviors
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests: