Skip to content

refactor: Refactor pytest from class based test to function based#3263

Closed
Zheaoli wants to merge 2 commits intoapache:mainfrom
Zheaoli:manjusaka/rename=pytest-name
Closed

refactor: Refactor pytest from class based test to function based#3263
Zheaoli wants to merge 2 commits intoapache:mainfrom
Zheaoli:manjusaka/rename=pytest-name

Conversation

@Zheaoli
Copy link
Copy Markdown
Member

@Zheaoli Zheaoli commented Oct 12, 2023

Refactor Python binding test from class-based test to function-based test.

It would improve config experience for the python binding test.

Also can support #3258

@Zheaoli Zheaoli marked this pull request as draft October 12, 2023 18:26
@github-actions github-actions Bot added the releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor" label Oct 12, 2023
@Zheaoli Zheaoli changed the title refactor: Refactor pytest name refactor: Refactor pytest from class based test to function based Oct 12, 2023
@Zheaoli Zheaoli marked this pull request as ready for review October 12, 2023 19:57
@Zheaoli Zheaoli force-pushed the manjusaka/rename=pytest-name branch from 5e0bccf to 6875d81 Compare October 12, 2023 20:12
Comment thread bindings/python/tests/test_services.py Outdated
class TestMemory(AbstractTestSuite):
service_name = "memory"

@pytest.fixture()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR, but the class way is preferred since we want to support pytest -vk memory,s3,fs which means we could construct more operators.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OKKKK,the function based can do the same thing, wait a liite bit

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the function based can do the same thing

I don't understand why we need to completely change our approach just to accomplish the same thing. Are there any other benefits of using function-based tests?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

CleanShot 2023-10-13 at 11 53 24@2x

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But having a class make it more clear about the tests belongings:

image

Signed-off-by: Manjusaka <me@manjusaka.me>
Signed-off-by: Manjusaka <me@manjusaka.me>
@Zheaoli Zheaoli force-pushed the manjusaka/rename=pytest-name branch from d08abce to 0bacb4c Compare October 13, 2023 03:54
@Xuanwo
Copy link
Copy Markdown
Member

Xuanwo commented Oct 13, 2023

I would rather close this PR as there hasn't been much improvement so far.

@Zheaoli Zheaoli closed this Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants