Let CI distinguish test compilation and test run failures #15600

mherrmann · 2022-10-21T19:33:24Z

Prerequisite for https://github.com/brave/devops/issues/8517.

At the moment, we have a single command for running tests: npm run test. This both builds and runs the tests. If the command fails, then it is unclear whether the failure was in building or running the tests. This leads to confusing error messages in our CI. The present PR is a preparation for fixing this.

This PR also incorporates a few small refactorings. Most notably, it introduces internal buidTests and runTests functions that better separate those concerns.

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

petemill

package.json changes seem fine. Please get another approval for the code changes before merge. I was only able to cursory review and whilst there are some minor issues (top-level anonymous functions and using let instead of const, that whole file has them and we need to improve the linter to catch these).

browser/brave_ads/ads_service_browsertest.cc

bridiver · 2022-11-09T22:31:39Z

build/commands/scripts/commands.js

+const buildTestsCommand = program.command('build_tests <suite>');
+const runTestsCommand = program.command('run_tests <suite>');
+
+[testCommand, buildTestsCommand, runTestsCommand].forEach((command) => {


I think this would be a lot simpler if we had an optional --target param for the build command. Separately we should do the same thing for create_dist

for backwards compat you change package.json for npm run test to handle this as two commands

I think this would be a lot simpler if we had an optional --target param for the build

We have such a param, don't we? For example:

npm run build -- --target=brave/components/policy:pack_policy_templates

I'm pretty sure I'm not fully understanding what you wrote. Could you clarify in more detail what you meant?

PS - as always ty for the review.

for backwards compat you change package.json for npm run test to handle this as two commands

I'm afraid I don't understand the message of this comment either. Yes I'm changing package.json in a way that's backwards compatible (while also retaining the test command for developer ease). Is there something I should change?

@bridiver could I ask you to clarify your comments?

Previously, the command exited with a non-zero code on both compilation and test errors. This made it impossible to determine which of the two happened - and thus hard to decide whether to generate test reports.

Previously, `npm run test --output` exited with a non-zero code on both compilation and test errors. This made it impossible to determine which of the two happened - and thus hard to decide whether to generate test reports. Now, the command exits with 0 on test failures. (Note that the behavior is unchanged when --output is not given. That is, in this case you still get non-zero exit codes for test failures.)

mherrmann self-assigned this Oct 21, 2022

mherrmann requested review from bridiver and a team as code owners October 21, 2022 19:33

mherrmann force-pushed the better-test-error-reporting branch 2 times, most recently from 004c5e1 to 15ce857 Compare October 31, 2022 06:41

mherrmann force-pushed the better-test-error-reporting branch 3 times, most recently from 2d35a52 to 3ef92d5 Compare November 8, 2022 05:16

petemill approved these changes Nov 8, 2022

View reviewed changes

mherrmann requested a review from a team as a code owner November 8, 2022 13:58

tmancey reviewed Nov 8, 2022

View reviewed changes

browser/brave_ads/ads_service_browsertest.cc Outdated Show resolved Hide resolved

bridiver reviewed Nov 9, 2022

View reviewed changes

mherrmann force-pushed the better-test-error-reporting branch from b42364d to 0879d7f Compare December 15, 2022 14:36

mherrmann added 5 commits December 19, 2022 18:31

Introduce npm commands build_/run_tests

195fcbd

Remove build_/run_tests npm commands

f10fc67

Exit npm run test --output with 0 if tests fail

a8ece7c

Previously, the command exited with a non-zero code on both compilation and test errors. This made it impossible to determine which of the two happened - and thus hard to decide whether to generate test reports.

Fix ReferenceError in test.js

d5a71b0

Fix typo

87e0af1

mherrmann force-pushed the better-test-error-reporting branch from cd8cd1a to 87e0af1 Compare December 19, 2022 17:31

mherrmann changed the title ~~Introduce separate build- and run_test commands~~ Let CI distinguish test-compilation and -run failures Dec 19, 2022

mherrmann changed the title ~~Let CI distinguish test-compilation and -run failures~~ Let CI distinguish test compilation and test run failures Dec 19, 2022

Fix a few small problems in npm run test

37ec204

mihaiplesa requested a review from bridiver December 23, 2022 10:21

bridiver approved these changes Jan 3, 2023

View reviewed changes

mherrmann added the release-notes/exclude label Jan 4, 2023

mherrmann merged commit 30be63f into master Jan 4, 2023

mherrmann deleted the better-test-error-reporting branch January 4, 2023 08:03

github-actions bot added this to the 1.48.x - Nightly milestone Jan 4, 2023

mherrmann added the QA/No label Jan 4, 2023

This was referenced Jan 4, 2023

Better test error reporting (uplift to 1.46.x) #16523

Merged

Better test error reporting (uplift to 1.47.x) #16524

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let CI distinguish test compilation and test run failures #15600

Let CI distinguish test compilation and test run failures #15600

mherrmann commented Oct 21, 2022 •

edited

Loading

petemill left a comment

bridiver Nov 9, 2022

bridiver Nov 9, 2022

mherrmann Nov 10, 2022 •

edited

Loading

mherrmann Nov 10, 2022

mherrmann Dec 5, 2022

Let CI distinguish test compilation and test run failures #15600

Let CI distinguish test compilation and test run failures #15600

Conversation

mherrmann commented Oct 21, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

petemill left a comment

Choose a reason for hiding this comment

bridiver Nov 9, 2022

Choose a reason for hiding this comment

bridiver Nov 9, 2022

Choose a reason for hiding this comment

mherrmann Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

mherrmann Nov 10, 2022

Choose a reason for hiding this comment

mherrmann Dec 5, 2022

Choose a reason for hiding this comment

mherrmann commented Oct 21, 2022 •

edited

Loading

mherrmann Nov 10, 2022 •

edited

Loading