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

Support brave-variations test seed testing process (uplift to 1.68.x) #24627

Merged
merged 1 commit into from
Jul 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ source_set("unit_tests") {
deps = [
"//base",
"//base/test:test_support",
"//brave/components/variations:buildflags",
"//chrome/browser",
"//chrome/browser/enterprise/connectors/analysis:features",
"//chrome/browser/ui",
Expand Down
19 changes: 2 additions & 17 deletions app/brave_main_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
#include "brave/components/constants/brave_switches.h"
#include "brave/components/speedreader/common/buildflags/buildflags.h"
#include "brave/components/update_client/buildflags.h"
#include "brave/components/variations/buildflags.h"
#include "brave/components/variations/command_line_utils.h"
#include "brave/renderer/brave_content_renderer_client.h"
#include "brave/utility/brave_content_utility_client.h"
#include "build/build_config.h"
Expand All @@ -35,7 +35,6 @@
#include "components/dom_distiller/core/dom_distiller_switches.h"
#include "components/embedder_support/switches.h"
#include "components/sync/base/command_line_switches.h"
#include "components/variations/variations_switches.h"
#include "google_apis/gaia/gaia_switches.h"

#if BUILDFLAG(IS_LINUX)
Expand Down Expand Up @@ -169,21 +168,7 @@ void BraveMainDelegate::AppendCommandLineOptions() {
brave_sync_service_url.c_str());
}

// Brave variations
if (!command_line->HasSwitch(variations::switches::kVariationsServerURL)) {
command_line->AppendSwitchASCII(variations::switches::kVariationsServerURL,
BUILDFLAG(BRAVE_VARIATIONS_SERVER_URL));
}
// Insecure fall-back for variations is set to the same (secure) URL. This is
// done so that if VariationsService tries to fall back to insecure url the
// check for kHttpScheme in VariationsService::MaybeRetryOverHTTP would
// prevent it from doing so as we don't want to use an insecure fall-back.
if (!command_line->HasSwitch(
variations::switches::kVariationsInsecureServerURL)) {
command_line->AppendSwitchASCII(
variations::switches::kVariationsInsecureServerURL,
BUILDFLAG(BRAVE_VARIATIONS_SERVER_URL));
}
variations::AppendBraveCommandLineOptions(*command_line);
}

std::optional<int> BraveMainDelegate::BasicStartupComplete() {
Expand Down
17 changes: 0 additions & 17 deletions app/brave_main_delegate_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,9 @@ TEST(BraveMainDelegateUnitTest, OverrideSwitchFromCommandLine) {
base::CommandLine& command_line = *base::CommandLine::ForCurrentProcess();
const std::string override_sync_url = "https://sync.com";
const std::string override_origin_trials_public_key = "public_key";
const std::string override_variations_url = "https://variations.com";
const std::string override_insecure_variations_url = "https://variations.com";
command_line.AppendSwitchASCII(syncer::kSyncServiceURL, override_sync_url);
command_line.AppendSwitchASCII(embedder_support::kOriginTrialPublicKey,
override_origin_trials_public_key);
command_line.AppendSwitchASCII(variations::switches::kVariationsServerURL,
override_variations_url);
command_line.AppendSwitchASCII(
variations::switches::kVariationsInsecureServerURL,
override_insecure_variations_url);

BraveMainDelegate::AppendCommandLineOptions();

Expand All @@ -66,14 +59,4 @@ TEST(BraveMainDelegateUnitTest, OverrideSwitchFromCommandLine) {
override_origin_trials_public_key.c_str(),
command_line.GetSwitchValueASCII(embedder_support::kOriginTrialPublicKey)
.c_str());
ASSERT_STREQ(
override_variations_url.c_str(),
command_line
.GetSwitchValueASCII(variations::switches::kVariationsServerURL)
.c_str());
ASSERT_STREQ(override_insecure_variations_url.c_str(),
command_line
.GetSwitchValueASCII(
variations::switches::kVariationsInsecureServerURL)
.c_str());
}
2 changes: 1 addition & 1 deletion browser/sources.gni
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ if (is_win && is_official_build) {
brave_chrome_browser_public_deps = [
"//brave/browser:browser_process",
"//brave/components/brave_sync:constants",
"//brave/components/variations:constants",
"//brave/components/variations",
]

if (is_mac) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,13 @@ base::Value::List GetVariationsList() {
}

base::Value::List variations_list;
for (std::string& variation : variations)
const std::string& seed_version = variations::GetSeedVersion();
if (!seed_version.empty() && seed_version != "1") {
variations_list.Append(seed_version);
}
for (std::string& variation : variations) {
variations_list.Append(std::move(variation));
}

return variations_list;
}
Expand Down
28 changes: 26 additions & 2 deletions components/variations/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,30 @@ buildflag_header("buildflags") {
flags = [ "BRAVE_VARIATIONS_SERVER_URL=\"$brave_variations_server_url\"" ]
}

group("constants") {
public_deps = [ ":buildflags" ]
static_library("variations") {
sources = [
"command_line_utils.cc",
"command_line_utils.h",
"switches.h",
]

deps = [
":buildflags",
"//base",
"//components/variations",
]
}

source_set("unit_tests") {
testonly = true

sources = [ "command_line_utils_unittest.cc" ]

deps = [
":buildflags",
":variations",
"//base",
"//components/variations",
"//testing/gtest",
]
}
63 changes: 63 additions & 0 deletions components/variations/command_line_utils.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Copyright (c) 2024 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

#include "brave/components/variations/command_line_utils.h"

#include <string>

#include "base/command_line.h"
#include "base/strings/string_util.h"
#include "brave/components/variations/buildflags.h"
#include "brave/components/variations/switches.h"
#include "components/variations/variations_switches.h"

namespace variations {

namespace {

// A GitHub workflow in the brave/brave-variations repository generates the test
// seed and uploads it to a URL with the following template, where $1 is the
// pull request number.
constexpr char kVariationsPrTestSeedUrlTemplate[] =
"https://griffin.brave.com/pull/$1/seed";

} // namespace

void AppendBraveCommandLineOptions(base::CommandLine& command_line) {
std::string variations_server_url = BUILDFLAG(BRAVE_VARIATIONS_SERVER_URL);

if (command_line.HasSwitch(switches::kVariationsPR)) {
variations_server_url = base::ReplaceStringPlaceholders(
kVariationsPrTestSeedUrlTemplate,
{command_line.GetSwitchValueASCII(switches::kVariationsPR)}, nullptr);

// Generated seed is not signed, so we need to disable signature check.
command_line.AppendSwitch(
variations::switches::kAcceptEmptySeedSignatureForTesting);

// Disable fetch throttling to force the fetch at startup on mobile
// platforms.
command_line.AppendSwitch(
variations::switches::kDisableVariationsSeedFetchThrottling);
}

if (!command_line.HasSwitch(variations::switches::kVariationsServerURL)) {
command_line.AppendSwitchASCII(variations::switches::kVariationsServerURL,
variations_server_url);
}

// Insecure fall-back for variations is set to the same (secure) URL. This
// is done so that if VariationsService tries to fall back to insecure url
// the check for kHttpScheme in VariationsService::MaybeRetryOverHTTP would
// prevent it from doing so as we don't want to use an insecure fall-back.
if (!command_line.HasSwitch(
variations::switches::kVariationsInsecureServerURL)) {
command_line.AppendSwitchASCII(
variations::switches::kVariationsInsecureServerURL,
variations_server_url);
}
}

} // namespace variations
21 changes: 21 additions & 0 deletions components/variations/command_line_utils.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright (c) 2024 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

#ifndef BRAVE_COMPONENTS_VARIATIONS_COMMAND_LINE_UTILS_H_
#define BRAVE_COMPONENTS_VARIATIONS_COMMAND_LINE_UTILS_H_

namespace base {
class CommandLine;
}

namespace variations {

// Appends Brave-specific command line options to fetch variations seed from the
// correct server.
void AppendBraveCommandLineOptions(base::CommandLine& command_line);

} // namespace variations

#endif // BRAVE_COMPONENTS_VARIATIONS_COMMAND_LINE_UTILS_H_
70 changes: 70 additions & 0 deletions components/variations/command_line_utils_unittest.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright (c) 2024 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

#include "brave/components/variations/command_line_utils.h"

#include <string>

#include "base/command_line.h"
#include "brave/components/variations/buildflags.h"
#include "brave/components/variations/switches.h"
#include "components/variations/variations_switches.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace variations {

TEST(VariationsCommandLineUtils, DefaultVariationsServerUrl) {
base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
AppendBraveCommandLineOptions(command_line);

EXPECT_EQ(command_line.GetSwitchValueASCII(
variations::switches::kVariationsServerURL),
BUILDFLAG(BRAVE_VARIATIONS_SERVER_URL));
EXPECT_EQ(command_line.GetSwitchValueASCII(
variations::switches::kVariationsInsecureServerURL),
BUILDFLAG(BRAVE_VARIATIONS_SERVER_URL));
EXPECT_FALSE(command_line.HasSwitch(
variations::switches::kAcceptEmptySeedSignatureForTesting));
EXPECT_FALSE(command_line.HasSwitch(
variations::switches::kDisableVariationsSeedFetchThrottling));
}

TEST(VariationsCommandLineUtils, OverrideVariationsServerUrl) {
base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
const std::string override_variations_url = "https://variations.com";
const std::string override_insecure_variations_url = "http://insecure.com";
command_line.AppendSwitchASCII(variations::switches::kVariationsServerURL,
override_variations_url);
command_line.AppendSwitchASCII(
variations::switches::kVariationsInsecureServerURL,
override_insecure_variations_url);
AppendBraveCommandLineOptions(command_line);

EXPECT_EQ(override_variations_url,
command_line.GetSwitchValueASCII(
variations::switches::kVariationsServerURL));
EXPECT_EQ(override_insecure_variations_url,
command_line.GetSwitchValueASCII(
variations::switches::kVariationsInsecureServerURL));
}

TEST(VariationsCommandLineUtils, SetVariationsPrParameter) {
base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
command_line.AppendSwitchASCII(variations::switches::kVariationsPR, "1234");
AppendBraveCommandLineOptions(command_line);

EXPECT_EQ(command_line.GetSwitchValueASCII(
variations::switches::kVariationsServerURL),
"https://griffin.brave.com/pull/1234/seed");
EXPECT_EQ(command_line.GetSwitchValueASCII(
variations::switches::kVariationsInsecureServerURL),
"https://griffin.brave.com/pull/1234/seed");
EXPECT_TRUE(command_line.HasSwitch(
variations::switches::kAcceptEmptySeedSignatureForTesting));
EXPECT_TRUE(command_line.HasSwitch(
variations::switches::kDisableVariationsSeedFetchThrottling));
}

} // namespace variations
18 changes: 18 additions & 0 deletions components/variations/switches.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright (c) 2024 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

#ifndef BRAVE_COMPONENTS_VARIATIONS_SWITCHES_H_
#define BRAVE_COMPONENTS_VARIATIONS_SWITCHES_H_

namespace variations::switches {

// If this flag is set to a brave/brave-variations pull request number, the
// browser will point "--variations-server-url" to a test seed URL from this
// pull request.
inline constexpr char kVariationsPR[] = "variations-pr";

} // namespace variations::switches

#endif // BRAVE_COMPONENTS_VARIATIONS_SWITCHES_H_
2 changes: 1 addition & 1 deletion ios/app/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ source_set("app") {
"//brave/components/p3a:buildflags",
"//brave/components/skus/browser",
"//brave/components/update_client:buildflags",
"//brave/components/variations:constants",
"//brave/components/variations",
"//brave/ios/app/resources",
"//brave/ios/browser",
"//brave/ios/browser/api/ads",
Expand Down
5 changes: 5 additions & 0 deletions ios/app/brave_core_switches.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ OBJC_EXPORT const BraveCoreSwitchKey BraveCoreSwitchKeyVModule;
///
/// Expected value: A URL string
OBJC_EXPORT const BraveCoreSwitchKey BraveCoreSwitchKeySyncURL;
/// Sets the variations URL to a test seed generated in brave/brave-variations
/// pull request.
///
/// Expected value: A brave/brave-variations pull request number
OBJC_EXPORT const BraveCoreSwitchKey BraveCoreSwitchKeyVariationsPR;
/// Overrides the variations seed URL. Defaults to production
///
/// Expected value: A URL string
Expand Down
3 changes: 3 additions & 0 deletions ios/app/brave_core_switches.mm
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "base/strings/sys_string_conversions.h"
#include "brave/components/brave_component_updater/browser/switches.h"
#include "brave/components/p3a/switches.h"
#include "brave/components/variations/switches.h"
#include "components/component_updater/component_updater_switches.h"
#include "components/sync/base/command_line_switches.h"
#include "components/variations/variations_switches.h"
Expand All @@ -23,6 +24,8 @@
base::SysUTF8ToNSString(switches::kVModule);
const BraveCoreSwitchKey BraveCoreSwitchKeySyncURL =
base::SysUTF8ToNSString(syncer::kSyncServiceURL);
const BraveCoreSwitchKey BraveCoreSwitchKeyVariationsPR =
base::SysUTF8ToNSString(variations::switches::kVariationsPR);
const BraveCoreSwitchKey BraveCoreSwitchKeyVariationsURL =
base::SysUTF8ToNSString(variations::switches::kVariationsServerURL);
// There is no exposed switch for rewards
Expand Down
16 changes: 2 additions & 14 deletions ios/app/brave_main_delegate.mm
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
#include "brave/components/brave_component_updater/browser/switches.h"
#include "brave/components/brave_sync/buildflags.h"
#include "brave/components/update_client/buildflags.h"
#include "brave/components/variations/buildflags.h"
#include "brave/components/variations/command_line_utils.h"
#include "components/browser_sync/browser_sync_switches.h"
#include "components/component_updater/component_updater_switches.h"
#include "components/sync/base/command_line_switches.h"
Expand Down Expand Up @@ -72,19 +72,7 @@ @implementation BundleLookupClass
BUILDFLAG(BRAVE_SYNC_ENDPOINT));
}

// Brave variations
if (!command_line->HasSwitch(variations::switches::kVariationsServerURL)) {
command_line->AppendSwitchASCII(variations::switches::kVariationsServerURL,
BUILDFLAG(BRAVE_VARIATIONS_SERVER_URL));

// Insecure fall-back for variations is set to the same (secure) URL. This
// is done so that if VariationsService tries to fall back to insecure url
// the check for kHttpScheme in VariationsService::MaybeRetryOverHTTP would
// prevent it from doing so as we don't want to use an insecure fall-back.
command_line->AppendSwitchASCII(
variations::switches::kVariationsInsecureServerURL,
BUILDFLAG(BRAVE_VARIATIONS_SERVER_URL));
}
variations::AppendBraveCommandLineOptions(*command_line);

if (!command_line->HasSwitch(switches::kVModule)) {
command_line->AppendSwitchASCII(switches::kVModule, "*/brave/*=0");
Expand Down
Loading
Loading