[CK_BUILDER] Add inline string diff for tests #3067
Conversation
kabrahamAMD
requested review from
a team,
ThomasNing,
afagaj,
andriy-ca,
aosewski,
aska-0096,
asleepzzz,
bartekxk,
carlushuang,
cgmillette,
coderfeli,
ddembeckAMD,
geyyer,
illsilin,
poyenc,
qianfengz,
shumway,
tenpercent and
vidyasagar-amd
as code owners
kabrahamAMD
force-pushed
the
kabraham/builder-inline-diff
branch
from
c3fe4ef to
18e8161
Compare
Snektron
changed the title
kabrahamAMD
force-pushed
the
kabraham/builder-inline-diff
branch
2 times, most recently
from
f6dffc6 to
c7d3403
Compare
| #include <ck/utility/sequence.hpp> | ||
|
|
||
| namespace ck_tile::builder { | ||
|
|
There was a problem hiding this comment.
It’s not clear to me why we are including this file in this PR. I don’t think we’re using any of it for inline_diff.
There was a problem hiding this comment.
No, I didn't mean the include. Why are we adding the file builder_utils.h in this PR?
There was a problem hiding this comment.
It's being included by test_builder_utils.cpp, which is using to_sequence_v, among others
There was a problem hiding this comment.
Removed it, see comment below
shumway
changed the title
| EXPECT_TRUE((std::is_same_v<result_seq, expected_seq>)); | ||
| } | ||
|
|
||
| TEST(BuilderUtils, StringLiteral) |
There was a problem hiding this comment.
Why is this test called BuilderUtils - StringLiteral? This is not what the test is. It shouldn't go in the file test_builder_utils.hpp, either.
The only files should be testing_utils.hpp, testing_utils.cpp, and the test for them, test_testing_utils.cpp. Builder utils don't belong here.
There was a problem hiding this comment.
Actually, it looks like my might not have pushed the latest changes you have. Your comments look like they may be ahead of the code in this PR.
There was a problem hiding this comment.
I think I just realized what happened here... I was looking at the diff vs. the branch you were pointing me to, and there was a test where strings were being compared. I assumed that was supposed to be the test for the inlineDiff function, and worked from there. But apparently this was the test for the builder_utils StringLiteral feature. That also explains the dependency on builder_utils from the two other tests in the comment above
…d builder_uitls, added non-color tests
kabrahamAMD
force-pushed
the
kabraham/builder-inline-diff
branch
from
5e7c9d4 to
9e744d5
Compare
Adds new testing functionality: an inline diff for string comparison.
Example usage:
EXPECT_THAT("Actual string", ck_tile::test::StringEqWithDiff("Expected string"));
Failure message:
Value of: "Actual string"
Expected: "Expected string"
Actual: "Actual string" (of type char [14]),
Diff: "[Expe|A]ct[ed|ual] string"
The inline-diff function uses the Wagner-Fischer algorithm to find the minimum edit distance and generate diff markers, which has O(N^2) complexity. It has optional color codes that are enabled with the matcher.
4 participants
This PR adds new testing functionality: an inline diff for string comparison.
Example usage:
EXPECT_THAT("Actual string", ck_tile::test::StringEqWithDiff("Expected string"));
Failure message:
Value of: "Actual string"
Expected: "Expected string"
Actual: "Actual string" (of type char [14]),
Diff: "[Expe|A]ct[ed|ual] string"
The inline-diff function uses the Wagner-Fischer algorithm to find the minimum edit distance and generate diff markers, which has O(N^2) complexity. It has optional color codes that are enabled with the matcher.