-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add manifest tests #80
Conversation
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.
Thanks @GeorgesLorre!
fondant/manifest.py
Outdated
@@ -149,6 +149,10 @@ def add_metadata(self, key: str, value: t.Any) -> None: | |||
def base_path(self) -> str: | |||
return self.metadata["base_path"] | |||
|
|||
@base_path.setter |
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.
If we add a setter specifically for base_path
instead of leveraging the add_metadata()
method, we probably should do this for the run_id
and component_id
as well?
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.
I used the add_metadata()
in my first version but decided against it since I was updating metadata and not adding it. But I guess we could use it instead of the setter.
def test_manifest_copy_and_adapt(valid_manifest): | ||
"""Test that a manifest can be copied and adapted without changing the original.""" | ||
manifest = Manifest(valid_manifest) | ||
new_manifest = manifest.copy() | ||
new_manifest.remove_subset("images") | ||
assert manifest._specification == valid_manifest | ||
assert new_manifest._specification != valid_manifest | ||
|
||
|
||
def test_no_validate_schema(monkeypatch, valid_manifest): |
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.
I don't really understand this test. When would this behavior occur?
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.
Probably never but we needed to cover this branch in the logic for coverage.
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.
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 message being raised mentions component_spec
instead of manifest
, so you might want to check that 😛
Expanded the tests to reach 100% coverage for all `manifest.py` code
Expanded the tests to reach 100% coverage for all
manifest.py
code