Changes to provide ‘npm run coverage’ #7924

masparrow · 2020-01-27T14:58:22Z

DO NOT MERGE

This PR is just to show how to utilise [Chromium Coverage]m on Mac OSX (Mojave) (https://chromium.googlesource.com/chromium/src/+/master/docs/testing/code_coverage.md) within Brave.

Warrants further discussion on how to incorporate it into dev usage, and CI/Automation with devops.

The following steps are needed, but are not production/devop platform final - we'd need to decide what/how best to fulfil the requirements.

Steps to 'be able to' run Coverage

Install llvm-profdata & llvm-cov
-- the install 'tool link' in Chromium's web page link didn't work for me, I obtained the full LLVM tools package from LLVM, and made sure the 'bin' folder was in my local PATH
Add the downloaded llvm tools bin to PATH
-- may not be necessary if installed properly
Add src/third_party/depot_tools to PATH
-- should already be required for Brave builds?… but I had to add it
Set an export LLVM_PROFILE_FILE='out/brave/report/brave_unit_tests.%4m.profraw' to restrict the profiles as per Chromium's 'Step 2 Create Raw Profiles'
Create src/Libraries
-- Nb. These steps could/should be better handled, but it's what I did to get this working. You may also need to have built with npm run build to produce these dylib's
-- Locate and copy libchallenge_bypass_ristretto.dylib to src/Libraries
-- Locate and copy libadblock.dylib to src/Libraries

Usage

npm run coverage

and fingers crossed, it should build brave_unit_tests and run coverage for the (temporarily) hardcoded folders cited in util.js

A link is given to the report index.html at the end of the analysis stage. For me, on an iMac Pro, this takes about 30-40 mins for a clean build.

The -f arguments within the utils.js step simply restrict the analysis to the given folders. Providing no filters runs across the whole brave_unit_tests target, and for me this took about an hour, and hit ~150GB storage space in total.

Suggestions

As noted in comments in util.js - passing arguments instead of hardcoding, but otherwise it's fairly simple.
I doubt we need a separate coverage.js but I'll leave that to devops/build guru's to decide :)

masparrow · 2020-01-27T15:00:25Z

lib/coverage.js

@@ -0,0 +1,108 @@
+const config = require('../lib/config')


This file coverage.js is a copy of build.js and is not strictly necessary, it just made sure I didn't break anything else whilst poking around.

The only noteworthy change compared to build.js is the utils.runCoverage step instead of utils.buildTarget

We could perhaps just have another arg for utils.buildTarget that goes on to do coverage, as the steps (as they are) require a full build I think... so it doesn't leverage any existing build of brave_unit_tests a local dev may have just done.

missed this comment the first time, but anyway the comments should hopefully help you clean it up

@bridiver cheers for the feedback :) So my thinking was building with npm run test -- brave_unit_tests should provide optional flags/args to also generate coverage profile data, provided it doesn't impact/cause issues elsewhere. WDYT?

masparrow · 2020-01-27T15:02:16Z

package.json

@@ -22,7 +22,8 @@
    "lint": "node ./scripts/commands.js lint",
    "test": "node ./scripts/commands.js test",
    "test:scripts": "jest lib scripts",
-    "test-security": "npm run audit_deps && node ./scripts/commands.js start --enable_brave_update --network_log --user_data_dir_name=brave-network-test"
+    "test-security": "npm run audit_deps && node ./scripts/commands.js start --enable_brave_update --network_log --user_data_dir_name=brave-network-test",
+    "coverage": "node ./scripts/commands.js coverage"


See above comments on coverage.js

masparrow · 2020-01-27T15:02:29Z

scripts/commands.js

@@ -18,6 +18,7 @@ const l10nDeleteTranslations = require('../lib/l10nDeleteTranslations')
 const createDist = require('../lib/createDist')
 const upload = require('../lib/upload')
 const test = require('../lib/test')
+const coverage = require('../lib/coverage')


See above comments on coverage.js

masparrow · 2020-01-27T15:02:50Z

scripts/commands.js

+  .option('--ninja <opt>', 'Additional Ninja command-line options, in the form <key>:<value>', collect, [])
+  .option('--brave_safetynet_api_key <brave_safetynet_api_key>')
+  .arguments('[build_config]')
+  .action(coverage)


Literally copy/paste of build - but changing the name to coverage

tmancey · 2020-01-28T10:55:48Z

Did you resolve the issue with the rust linkage?

masparrow · 2020-01-28T13:34:28Z

Did you resolve the issue with the rust linkage?

Noted the necessary inclusions in the description, looking for feedback on best approach for them.

bridiver · 2020-01-28T23:12:54Z

lib/coverage.js

+const path = require('path')
+const fs = require('fs-extra')
+
+const touchOverriddenFiles = () => {


why are we duplicating this method?

bridiver · 2020-01-28T23:13:31Z

lib/coverage.js

+  }
+
+  if (config.xcode_gen_target) {
+    util.generateXcodeWorkspace()


why would coverage generate the xcode workspace?

bridiver · 2020-01-28T23:14:23Z

lib/coverage.js

+  config.update(options)
+  checkVersionsMatch()
+
+  touchOverriddenFiles()


actually why are any of these here? If coverage depends on build then it should run build and the coverage

bridiver · 2020-01-28T23:14:44Z

scripts/commands.js

+  .option('--target_os <target_os>', 'target OS')
+  .option('--target_arch <target_arch>', 'target architecture')
+  .option('--target_apk_base <target_apk_base>', 'target Android OS apk (classic, modern, mono)', 'classic')
+  .option('--android_override_version_name <android_override_version_name>', 'Android version number')


I don't think most of these options apply to coverage

bridiver · 2020-01-28T23:18:54Z

lib/util.js

+    const args = util.buildArgsToString(config.buildArgs())
+    // provide arg for COVERAGE_OUT_DIR
+    util.run('gn', ['gen',
+      'out/brave/coverage', // make an arg - i.e. build with COVERAGE_OUT_DIR as 'out/COVERAGE_OUT_DIR/coverage'


I don't think we want to run a new build for code coverage, this doesn't seem right to me

bridiver · 2020-01-28T23:20:41Z

lib/util.js

+    // provide arg for COVERAGE_OUT_DIR
+    util.run('gn', ['gen',
+      'out/brave/coverage', // make an arg - i.e. build with COVERAGE_OUT_DIR as 'out/COVERAGE_OUT_DIR/coverage'
+      '--args="' + args + ' use_clang_coverage=true is_component_build=false dcheck_always_on=true"'], options)


this is definitely not right, these should always be handled by config.js and you shouldn't be running gn gen directly like this anyway, you should be using util.buildTarget, but I'm not sure that makes sense here either since I think we should just be running build first?

bridiver · 2020-01-28T23:21:03Z

lib/util.js

+  runCoverage: (options = config.defaultOptions) => {
+    console.log('running coverage for ' + config.buildTarget + '...')
+
+    if (process.platform === 'win32') util.updateOmahaMidlFiles()


we definitely should not be copying all this logic from build


        Changes to provide ‘npm run coverage’

- using Chromium coverage.py Resolves #7750

masparrow reviewed Jan 27, 2020

View changes

masparrow self-assigned this Jan 27, 2020

masparrow added the feature/dev-tools label Jan 27, 2020

masparrow force-pushed the issues/7750 branch from e8af854 to f958c49 Jan 27, 2020

masparrow added the CI/skip label Jan 28, 2020

masparrow requested review from diracdeltas, bridiver and bsclifton Jan 28, 2020

bridiver reviewed Jan 28, 2020

View changes

lib/coverage.js

const path = require('path')

const fs = require('fs-extra')

const touchOverriddenFiles = () => {

This comment has been minimized.

Sign in to view

Copy link

bridiver Jan 28, 2020
Collaborator

why are we duplicating this method?

bridiver reviewed Jan 28, 2020

View changes

lib/coverage.js

}

if (config.xcode_gen_target) {

util.generateXcodeWorkspace()

This comment has been minimized.

Sign in to view

Copy link

bridiver Jan 28, 2020
Collaborator

why would coverage generate the xcode workspace?

bridiver reviewed Jan 28, 2020

View changes

lib/coverage.js

config.update(options)

checkVersionsMatch()

touchOverriddenFiles()

This comment has been minimized.

Sign in to view

Copy link

bridiver Jan 28, 2020
Collaborator

actually why are any of these here? If coverage depends on build then it should run build and the coverage

bridiver reviewed Jan 28, 2020

View changes

masparrow force-pushed the issues/7750 branch 2 times, most recently from a6dc34e to a7f4413 Feb 5, 2020

masparrow force-pushed the issues/7750 branch 3 times, most recently from 62ab5ac to b92e93e Feb 17, 2020

Changes to provide ‘npm run coverage’

Loading status checks…

76524f7

- using Chromium coverage.py Resolves #7750

masparrow force-pushed the issues/7750 branch from b92e93e to 76524f7 Mar 17, 2020

brave / brave-browser

Changes to provide ‘npm run coverage’ #7924

Changes to provide ‘npm run coverage’ #7924

masparrow commented Jan 27, 2020 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tmancey commented Jan 28, 2020

masparrow commented Jan 28, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brave / brave-browser

Join GitHub today

Changes to provide ‘npm run coverage’ #7924

Changes to provide ‘npm run coverage’ #7924

Conversation

masparrow commented Jan 27, 2020 • edited

Steps to 'be able to' run Coverage

Usage

Suggestions

This comment has been minimized.

masparrow Jan 27, 2020 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tmancey commented Jan 28, 2020

masparrow commented Jan 28, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

masparrow commented Jan 27, 2020 •

edited

masparrow Jan 27, 2020 •

edited