Skip to content

[test] Rename tests according to the convention#152

Merged
mhs4670go merged 1 commit intoSamsung:mainfrom
mhs4670go:rel
Jun 18, 2025
Merged

[test] Rename tests according to the convention#152
mhs4670go merged 1 commit intoSamsung:mainfrom
mhs4670go:rel

Conversation

@mhs4670go
Copy link
Contributor

This commit renames tests according to the convention.

Related: #151
TICO-DCO-1.0-Signed-off-by: seongwoo mhs4670go@naver.com

This commit renames tests according to the convention.

TICO-DCO-1.0-Signed-off-by: seongwoo <mhs4670go@naver.com>
@mhs4670go mhs4670go requested a review from a team June 17, 2025 07:05
Copy link

@Seunghui98 Seunghui98 left a comment

Choose a reason for hiding this comment

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

LGTM!


class NotMergedConsecutiveCatTest1(SinglePassValueTest):
def test_pass(self):
def test_pass_neg(self):
Copy link
Contributor

@miusic miusic Jun 17, 2025

Choose a reason for hiding this comment

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

May I ask why this test is marked as negative?
I guess that this test is more fit to the positive test according to the convention in #151

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the net failed to meet the condition of the pass. Can we really say it is a positive test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, let's clarify the comment for the readers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. 🤔 then, how about to check a condition like below after the pass is run?

self.assertNotEqual(num_of_ops(self.exported_program(), ops.aten.cat), 1)

Copy link
Contributor

Choose a reason for hiding this comment

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

Above suggestion is just an optional one
I understand your intention thanks to your comment 😄 #152 (comment)


class NotMergedConsecutiveCatTest2(SinglePassValueTest):
def test_pass(self):
def test_pass_neg(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@seockho-kim seockho-kim left a comment

Choose a reason for hiding this comment

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

LGTM

@mhs4670go mhs4670go requested a review from miusic June 18, 2025 02:34
Copy link
Contributor

@miusic miusic left a comment

Choose a reason for hiding this comment

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

LGTM with minor comment 😄 #152 (comment)

@mhs4670go mhs4670go merged commit 3b3efea into Samsung:main Jun 18, 2025
5 checks passed
@mhs4670go mhs4670go deleted the rel branch June 19, 2025 02:26
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.

4 participants