-
Notifications
You must be signed in to change notification settings - Fork 64
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
Fix fetch_runs_table
method for state param usage
#1253
Conversation
Codecov ReportBase: 73.59% // Head: 71.37% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1253 +/- ##
==========================================
- Coverage 73.59% 71.37% -2.23%
==========================================
Files 281 281
Lines 13573 13585 +12
==========================================
- Hits 9989 9696 -293
- Misses 3584 3889 +305
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
CHANGELOG.md
Outdated
@@ -21,6 +21,7 @@ | |||
- Added exception for unsupported types ([#1229](https://github.com/neptune-ai/neptune-client/pull/1229)) | |||
- Change run status to Active / Inactive ([#1233](https://github.com/neptune-ai/neptune-client/pull/1233)) | |||
- Package renamed from `neptune-client` to `neptune` ([#1225](https://github.com/neptune-ai/neptune-client/pull/1225)) | |||
- Changed return values for run state ([#1253](https://github.com/neptune-ai/neptune-client/pull/1253)) |
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.
Let's change it to something about changing values for filtering runs table with the state. + Let's update the old one (Line 22) to something about returned values from the runs table state value.
tests/unit/neptune/new/conftest.py
Outdated
|
||
@pytest.fixture(scope="session") | ||
def project() -> Project: | ||
return init_project(mode="read-only") |
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.
No need for this. debug
+ mock backend
assert not runs.empty | ||
assert tag in runs["sys/tags"].values | ||
|
||
def test_fetch_runs_table_with_incorrect_states(self, project): |
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.
This could be just a unittest 😉. Check AbstractTablesTestMixin
@@ -155,3 +156,25 @@ def get_model_versions_as_rows(**kwargs): | |||
return container.fetch_model_versions_table(**kwargs).to_rows() | |||
|
|||
self._test_fetch_from_container(init_run, get_model_versions_as_rows) | |||
|
|||
@pytest.mark.parametrize( | |||
"state_active,state_inactive", [("active", "inactive"), ("Active", "Inactive"), ("aCTive", "inAcTiVE")] |
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.
Same, let's just try to filter from API with some "new" value for state in e2e. In unittests -> Check that is case insensitive.
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.
Looks good, thanks!
Before submitting checklist