-
Notifications
You must be signed in to change notification settings - Fork 15
fix: Preserve existing config.toml entries that are not managed by this tool #39
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Format this file with flake8. CI is failing on this test specifically colcon-ros-cargo/test/test_flake8.py Line 12 in dc26ba5
I think this answers your question in the description as well
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| import pytest | ||
| from pathlib import Path | ||
| import toml | ||
| import os | ||
|
|
||
| from colcon_ros_cargo.task.ament_cargo.build import write_cargo_config_toml | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def temp_workspace(tmp_path): | ||
| """Create a temporary workspace directory.""" | ||
| original_cwd = os.getcwd() | ||
| os.chdir(tmp_path) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fixture expects a >>> import os
>>> os.chdir(None)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: chdir: path should be string, bytes, os.PathLike or integer, not NoneType
>>> os.chdir("")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
FileNotFoundError: [Errno 2] No such file or directory: '' |
||
| yield tmp_path | ||
| os.chdir(original_cwd) | ||
|
|
||
|
|
||
| def test_write_cargo_config_toml_creates_new_file(temp_workspace): | ||
| """Test that config.toml is created when it doesn't exist.""" | ||
| package_paths = { | ||
| "my_package": Path("/path/to/my_package"), | ||
| "other_package": Path("/path/to/other_package"), | ||
| } | ||
|
|
||
| write_cargo_config_toml(package_paths) | ||
|
|
||
| config_file = temp_workspace / ".cargo" / "config.toml" | ||
| assert config_file.exists() | ||
|
|
||
| with config_file.open("r") as f: | ||
| content = toml.load(f) | ||
|
|
||
| assert "patch" in content | ||
| assert "crates-io" in content["patch"] | ||
| assert content["patch"]["crates-io"]["my_package"] == { | ||
| "path": "/path/to/my_package" | ||
| } | ||
| assert content["patch"]["crates-io"]["other_package"] == { | ||
| "path": "/path/to/other_package" | ||
| } | ||
|
|
||
|
|
||
| def test_write_cargo_config_toml_merges_entries(temp_workspace): | ||
| """Test that the it merges its changes and preserves existing config.""" | ||
| config_dir = temp_workspace / ".cargo" | ||
| config_dir.mkdir(exist_ok=True) | ||
| config_file = config_dir / "config.toml" | ||
|
|
||
| existing_content = { | ||
| "build": { | ||
| "target": "x86_64-unknown-linux-gnu", | ||
| "jobs": 4, | ||
| }, | ||
| "patch": { | ||
| "crates-io": { | ||
| "existing_package": {"path": "/existing/path"}, | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| with config_file.open("w") as f: | ||
| toml.dump(existing_content, f) | ||
|
|
||
| package_paths = { | ||
| "new_package": Path("/path/to/new_package"), | ||
| } | ||
|
|
||
| write_cargo_config_toml(package_paths) | ||
|
|
||
| with config_file.open("r") as f: | ||
| content = toml.load(f) | ||
|
|
||
| # Existing config is preserved | ||
| assert content["build"]["target"] == "x86_64-unknown-linux-gnu" | ||
| assert content["build"]["jobs"] == 4 | ||
|
|
||
| # Old crates-io patch is removed | ||
| assert "existing_package" not in content["patch"]["crates-io"] | ||
|
|
||
| # New crates-io patch is present | ||
| assert content["patch"]["crates-io"]["new_package"] == { | ||
| "path": "/path/to/new_package" | ||
| } | ||
|
|
||
|
|
||
| def test_write_cargo_config_toml_updates_existing_patch(temp_workspace): | ||
| """Test that updating an existing patch overwrites it.""" | ||
| config_dir = temp_workspace / ".cargo" | ||
| config_dir.mkdir(exist_ok=True) | ||
| config_file = config_dir / "config.toml" | ||
|
|
||
| existing_content = { | ||
| "patch": { | ||
| "crates-io": { | ||
| "my_package": {"path": "/old/path"}, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| with config_file.open("w") as f: | ||
| toml.dump(existing_content, f) | ||
|
|
||
| package_paths = { | ||
| "my_package": Path("/new/path"), | ||
| } | ||
|
|
||
| write_cargo_config_toml(package_paths) | ||
|
|
||
| with config_file.open("r") as f: | ||
| content = toml.load(f) | ||
|
|
||
| assert content["patch"]["crates-io"]["my_package"] == {"path": "/new/path"} | ||
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.
Should we wrap this in a
try exceptso the tool doesn't crash if the file being loaded is malformed?