Skip to content
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

List all sets: new get_set endpoint and test reductions #86

Open
wants to merge 1 commit into
base: list_all_sets_begone_foul_redundant_classes
Choose a base branch
from

Conversation

ialarmedalien
Copy link
Collaborator

@ialarmedalien ialarmedalien commented Dec 18, 2023

Highlights from this PR:

  • Add new get_set_v1 generic set retrieval endpoint.
  • Remove A TON of redundancy from the tests.
  • Move tests to more sensible places.
  • Generally tidy things up.
  • A MASSIVE line count difference thanks to git's crap diff algorithm.

…redundancy from the tests. Move tests to more sensible places. Generally tidy things up.
@ialarmedalien ialarmedalien requested a review from briehl December 18, 2023 17:49
@ialarmedalien ialarmedalien self-assigned this Dec 18, 2023
Comment on lines +102 to +103
funcdef get_set_v1(GetSetV1Params params)
returns (GetGenericSetV1Result result) authentication optional;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exciting new function

the DifferentialExpressionMatrixSet.
ref_path is optionally returned by get_differential_expression_matrix_set_v1()
when its input parameter 'include_set_item_ref_paths' is set to 1.
You should never set 'info'; it is generated by the workspace.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edited this generic blurb to be a bit more coherent

typedef structure {
string description;
list<DifferentialExpressionMatrixSetItem> items;
} DifferentialExpressionMatrixSet;

/*
ref - workspace reference to DifferentialExpressionMatrixSet object.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this description is up at the top of the file - no need to repeat it

Comment on lines +58 to +63
def _get_set_v1(
self: "SetAPI", ctx: dict[str, Any], params: dict[str, Any]
) -> dict[str, Any]:
ws = Workspace(self.workspace_url, token=ctx["token"])
set_interface = SetInterfaceV1(ws)
return set_interface.get_set(params)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the get_***_set_v1 queries now use this method under the hood

# END_CONSTRUCTOR
pass


def get_differential_expression_matrix_set_v1(self, ctx, params):
def get_set_v1(self, ctx, params):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just love the way that git has done the diff here.

# ctx is the context object
# return variables are: result
# BEGIN get_differential_expression_matrix_set_v1
result = self._get_set_v1(ctx, params)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the get_***_set_v1 queries now look like this

[("default_ws_id", data_fixture) for data_fixture in SET_FIXTURE_MAP],
indirect=["ws_id", "data_fixture"],
)
def test_save_set(ws_id: int, data_fixture: dict[str, Any]) -> None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this over from common_test.py

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (82de8b9) 65.21% compared to head (fcd47c5) 65.03%.

Additional details and impacted files
@@                               Coverage Diff                               @@
##           list_all_sets_begone_foul_redundant_classes      #86      +/-   ##
===============================================================================
- Coverage                                        65.21%   65.03%   -0.19%     
===============================================================================
  Files                                               12       12              
  Lines                                             1124     1121       -3     
===============================================================================
- Hits                                               733      729       -4     
- Misses                                             391      392       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


@pytest.mark.parametrize("ws_id", ["default_ws_id"], indirect=["ws_id"])
@pytest.mark.parametrize("set_name", list(SET_FIXTURE_MAP))
def test_get_set_all_set_types(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a new test - runs through all the different set types and makes sure they can all be fetched.

"""Test set retrieval for with a variety of parameter configurations.

This uses `random_fixture` to pick one of the set types and run through the different combinations of
parameters that can be used for a `get_set_v1` query.
Copy link
Collaborator Author

@ialarmedalien ialarmedalien Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was in SetInterface_test.py but moved it here after adding the generic get_set endpoint to the API.

The core tests for this codebase are now:

  • test a save query for each set type
  • test get_set with a random set and every combination of input params
  • test get_***_set for each set type and one combination of input params

The two different get_set tests should ensure that get_set is thoroughly tested without requiring seven hundred zillion individual tests.

from SetAPI.generic.SetInterfaceV1 import SetInterfaceV1

STANDARD_VALUES = [None, 0, 1]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to SetAPI_test.py

@@ -1,5 +1,4 @@
"""General tests to be performed on most or all classes."""
from test.conftest import SET_FIXTURE_MAP
"""Sets of assertions for testing the saving and retrieval of sets."""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now just functions to check that the output of save and get queries is as expected -- no actual tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant