Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable BuildTimeUnittest #2066

Merged

Conversation

sherryzy
Copy link
Contributor

@sherryzy sherryzy commented Dec 11, 2023

b/298237650
b/316012657

@@ -28,4 +28,7 @@ if (is_official_build) {
# changes to keep this up to date. (Bots run gn on each build, and for devs
# the timestamp being 100% accurate doesn't matter.)
# See compute_build_timestamp.py for tradeoffs for picking the timestamp.
build_timestamp = "123"
build_timestamp = exec_script(compute_build_timestamp,
Copy link
Contributor

Choose a reason for hiding this comment

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

In order for reproducible builds to work, we don't want to put a timestamp in our build—if the test is solely checking this build_timestamp value, I think we should disable the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is what the test is trying to test.

we don't want to put a timestamp in our build

So is this a permanently or temporarily decision? If we just don't want to put a timestamp in our build, I agree we can just disable the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a permanent decision, at least for now. Let's just disable the test

@sherryzy sherryzy changed the title Fix BuildTime.DateLooksValid test failure Disable BuildTimeUnittest Dec 12, 2023
base/BUILD.gn Outdated
@@ -3132,7 +3132,8 @@ test("base_unittests") {
"big_endian_unittest.cc",
"bit_cast_unittest.cc",
"bits_unittest.cc",
"build_time_unittest.cc",
# Cobalt builds don't include build timestamps.
# "build_time_unittest.cc",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should either have a conditional checking use_cobalt_customizations that removes the entire source file if we don't want any of it or we should add the DISABLED_ gtest macro if we're disabling just certain tests in the file, that way we can have easier ways of tracking disabled tests

base/BUILD.gn Outdated
if (use_cobalt_customizations) {
sources += [
# Cobalt builds don't include build timestamps.
# "build_time_unittest.cc",
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the confusion, I meant to leave it in the original list of sources and to add a sources -= here to be clear about which files we don't compile

@sherryzy sherryzy merged commit f76d02b into youtube:feature/all-upstream-update Dec 13, 2023
91 of 117 checks passed
@sherryzy sherryzy deleted the base_unittest branch December 13, 2023 19:17
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.

2 participants