Skip to content

Remove DefaultTimeConverterIsIdentity test#1401

Closed
ddelgadovargas-cyber wants to merge 2 commits into
pytorch:mainfrom
ddelgadovargas-cyber:fix_approximate_clock_test
Closed

Remove DefaultTimeConverterIsIdentity test#1401
ddelgadovargas-cyber wants to merge 2 commits into
pytorch:mainfrom
ddelgadovargas-cyber:fix_approximate_clock_test

Conversation

@ddelgadovargas-cyber
Copy link
Copy Markdown
Contributor

@ddelgadovargas-cyber ddelgadovargas-cyber commented May 13, 2026

The DefaultTimeConverterIsIdentity test in ApproximateClockTest.cpp was failing because it relies on the state of a global static variable returned by get_time_converter(). If other tests in the same binary run first and modify this state, the test fails because the converter is no longer the identity function.

third_party/libkineto/test/ApproximateClockTest.cpp:105: Failure
Expected equality of these values:
  converter(kTestValue)
    Which is: 0
  static_cast<libkineto::time_t>(kTestValue)
    Which is: 123456789
Stack trace:
0x7fb663e19a5a: ApproximateClockTest_DefaultTimeConverterIsIdentity_Test::TestBody() @ ??:??
0x7fb5f80e4086: testing::Test::Run() @ ??:??
0x7fb5f80e50db: testing::TestInfo::Run() @ ??:??
... Google Test internal frames ...

This test validates the identity function in its initial state, which is not very useful. This change removes the test case.

@meta-cla meta-cla Bot added the cla signed label May 13, 2026
Comment thread libkineto/test/ApproximateClockTest.cpp Outdated

TEST(ApproximateClockTest, DefaultTimeConverterIsIdentity) {
auto& converter = get_time_converter();
converter = [](approx_time_t t) { return t; };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Digging into this, I think we should just remove this test entirely. get_time_converter() being the identity function is its initial state that we expect to be modified in:

get_time_converter() = clockConverter.makeConverter();
This test, then, is not actually testing anything useful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@scotts , I updated the PR header and removed the test case.

@ddelgadovargas-cyber ddelgadovargas-cyber changed the title Reset get_time_converter() to identity in ApproximateClockTest Remove DefaultTimeConverterIsIdentity test May 14, 2026
@meta-codesync
Copy link
Copy Markdown

meta-codesync Bot commented May 14, 2026

@scotts has imported this pull request. If you are a Meta employee, you can view this in D105224372.

@meta-codesync
Copy link
Copy Markdown

meta-codesync Bot commented May 15, 2026

@scotts merged this pull request in 81d31cd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants