Use pytest mark to run subset of integ modin tests in Snowfort #1703
Conversation
sfc-gh-lmukhopadhyay
added
NO-CHANGELOG-UPDATES
Signed-off-by: Labanya Mukhopadhyay <[email protected]>
Signed-off-by: Labanya Mukhopadhyay <[email protected]>
Signed-off-by: Labanya Mukhopadhyay <[email protected]>
sfc-gh-lmukhopadhyay
force-pushed
the
lmukhopadhyay-SNOW-1447077
branch
from
7723397 to
2dcc3e9
Compare
Signed-off-by: Labanya Mukhopadhyay <[email protected]>
Signed-off-by: Labanya Mukhopadhyay <[email protected]>
Signed-off-by: Labanya Mukhopadhyay <[email protected]>
sfc-gh-lmukhopadhyay
force-pushed
the
lmukhopadhyay-SNOW-1447077
branch
from
1fc6794 to
3e33d53
Compare
Signed-off-by: Labanya Mukhopadhyay <[email protected]>
Signed-off-by: Labanya Mukhopadhyay <[email protected]>
Signed-off-by: Labanya Mukhopadhyay <[email protected]>
sfc-gh-lmukhopadhyay
requested review from
sfc-gh-evandenberg,
sfc-gh-helmeleegy,
sfc-gh-jdu,
sfc-gh-yixie and
sfc-gh-jrose
Signed-off-by: Labanya Mukhopadhyay <[email protected]>
|
@sfc-gh-lmukhopadhyay can you update the description of this pr to include how the mark will be used when we run the tests? basically how server side uses these marks? |
| @@ -603,6 +603,7 @@ def g(v): | |||
| ] | |||
|
|
|||
|
|
|||
| @pytest.mark.modin_sp_short_regress | |||
There was a problem hiding this comment.
this test seems simply raises an NotImplementedError, should we at least get one test point that runs?
There was a problem hiding this comment.
right this one does not need to be in.
| @@ -37,6 +37,7 @@ def __init__(self, sp_df: OrderedDataFrame, internal: InternalFrame) -> None: | |||
| self.internal_frame: InternalFrame = internal | |||
|
|
|||
|
|
|||
| @pytest.mark.modin_sp_short_regress | |||
There was a problem hiding this comment.
is it possible to simply mark the whole suite to run as short_regress?
There was a problem hiding this comment.
Yes, but right now we try to be clear this is only used by modin. Today python API tests are defined in the snowfort code.
There was a problem hiding this comment.
I think my question is that is it possible to mark the whole suite with one marker instead of marking it one after another? Not about the name, we can keep modin in the name
There was a problem hiding this comment.
Yes should be possible, we can add this in the next marker PR
| @@ -196,6 +196,8 @@ markers = | |||
| timeout: tests that need a timeout time | |||
| localtest: local tests | |||
| scala: scala tests | |||
| modin_sp_short_regress: modin short_regress tests run in sproc | |||
There was a problem hiding this comment.
add a comment here to make it clear that those marker are used at server side to decide which tests to run, any change to the name will require a server change also, please be careful with any changes here
There was a problem hiding this comment.
Is this obvious? If marker changed, it definitely will affect server side.
There was a problem hiding this comment.
I don't think it is obvious, this is client side change, it is just used to run the tests, can be at both client and server. If any change is made at client side, server side test will not automatically run, which may not be captured immediately.
It would be better if we can add a comment here to make it more clear to all developer in Snowpark python
There was a problem hiding this comment.
I see, we can add more comments to make it clear.
| @@ -603,6 +603,7 @@ def g(v): | |||
| ] | |||
|
|
|||
|
|
|||
| @pytest.mark.modin_sp_short_regress | |||
| @pytest.mark.parametrize("data, apply_func", TRANSFORM_DATA_FUNC_MAP) | |||
| @sql_count_checker(query_count=0) | |||
There was a problem hiding this comment.
Can you also get one from groupby apply?
There was a problem hiding this comment.
Now groupby apply and some apply tests are failing. We will add it in the coming PRs if we can fix those tests.
There was a problem hiding this comment.
Yes, @sfc-gh-lmukhopadhyay mentioned about this to me, she will tickets for the each category of test failure for tracking
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1447077
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
We are using pytest.mark to annotate tests that are selected to be run in stored procedures with
short_regressorprecommit. The annotations are used to run the tests server side in the Snowfort regression framework either as part of the snowflake/main merge gate (short_regress) or the precommit run. For example to trigger the short_regress tests, the server side Snowfort test runspytest.main(['filedir', '-m', 'short_regress').More details can be found in the design doc here.