IcuBridge migration 10.1/n: services/tracing [chromium/src : main]
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
YMDT::Short()Similar question as before, this appears to me like a change in the format. Could you verify? And correct the format? Perhaps add a test to the unittest that ensure `kTraceCaptureDatetimeKey` has the proper format?
Related details
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi added 1 comment![]( "Open in Gerrit")
Similar question as before, this appears to me like a change in the format. Could you verify? And correct the format? Perhaps add a test to the unittest that ensure `kTraceCaptureDatetimeKey` has the proper format?
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 6 comments![]( "Open in Gerrit")
Also looks like other files crept in as well.
TRACE_TIME_NOW().UTCExplode(&exploded);explode can apparently fail I see? Perhaps add a
`CHECK(exploded.HasValidValues());`
TRACE_TIME_NOW(), "y-M-d H:m:s", icu::TimeZone::getGMT()),Generally GMT == UTC, is that correct here as well? I think so, but would be nice to have a passing test before the modification.
base::StringPrintf("%d-%d-%d %d:%d:%d", exploded.year, exploded.month,this was lower case y, which is just last two digits, is exploded also just for the last two digits? Looks like no from my code reading and the tests?
MetadataDataSource::AddMetadataToBundle("trace-capture-datetime",
datetime_str, bundle.get());This just ensures that the string matches the current inserted format, not that the code in WriteMetadata outputs this time format.
You'll want to switch this into a similar format as `RecordAndroidMetadata`. I.E. move the logic into a helper function accessible and then test that output.
EXPECT_EQ(metadata.string_value().ToStdString(), "2016-8-18 22:28:10");Can you ensure this test was passing before your change? Ideally you'd actually submit the test before your change. I suspect this would fail before the change.
Related details
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi added 5 comments![]( "Open in Gerrit")
Also looks like other files crept in as well.
Done
explode can apparently fail I see? Perhaps add a
`CHECK(exploded.HasValidValues());`
Done
base::StringPrintf("%d-%d-%d %d:%d:%d", exploded.year, exploded.month,this was lower case y, which is just last two digits, is exploded also just for the last two digits? Looks like no from my code reading and the tests?
lower case 'y' does not always translate to last two digits, in "en-US" most of the time, a single 'y' will translate to the four-digit year format.
MetadataDataSource::AddMetadataToBundle("trace-capture-datetime",
datetime_str, bundle.get());This just ensures that the string matches the current inserted format, not that the code in WriteMetadata outputs this time format.
You'll want to switch this into a similar format as `RecordAndroidMetadata`. I.E. move the logic into a helper function accessible and then test that output.
Done
EXPECT_EQ(metadata.string_value().ToStdString(), "2016-8-18 22:28:10");Can you ensure this test was passing before your change? Ideally you'd actually submit the test before your change. I suspect this would fail before the change.
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
1 comment
EXPECT_EQ(metadata.string_value().ToStdString(), "2016-8-18 22:28:10");Danilo TedeschiCan you ensure this test was passing before your change? Ideally you'd actually submit the test before your change. I suspect this would fail before the change.
Done
Huh I though [lowercase y](https://man7.org/linux/man-pages/man1/date.1.html#:~:text=%25y%20%20%20%20%20last%20two%20digits%20of%20year%20(ambiguous%3B%2000..99)) was last 2 digits, but indeed it seems like it was a full year in this case.
Thanks for double checking for me!
Related details
- Danilo Tedeschi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danilo Tedeschi (Gerrit)
Danilo Tedeschi voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
IcuBridge migration 10.1/n: services/tracing
Migrate deprecated UnlocalizedTimeFormatWithPattern and
LocalizedTimeFormatWithPattern usages to use
base::i18n::IcuBridge::DateTimeFormatter and modern time APIs.
- M services/tracing/public/cpp/BUILD.gn
- M services/tracing/public/cpp/perfetto/metadata_data_source.cc
- M services/tracing/public/cpp/perfetto/metadata_data_source.h
- M services/tracing/public/cpp/perfetto/metadata_data_source_unittest.cc
Code-Review: +1 by Stephen Nusko
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |