electron/bugbot

Making life easier for people who report or triage Electron issues.

BugBot

Overview

1. People file Electron bug reports with tests.
2. BugBot can run the test on many platforms and Electron versions, then report back in-issue.
3. BugBot can bisect regressions down to a range of commits, then report back in-issue.
4. Users and maintainers can see where the bug is reproducible.

Usage

By bug reporters

BugBot only needs one thing: a test that exits with exitCode 0 if Electron works, or nonzero if Electron has a bug.

One easy way to make a test is with Electron Fiddle. Its "File > New Test" has a test template with some lightweight test helpers and its "Publish" button can upload the test to the gist.github.com pastebin.

If you want to do it manually, the only requirement is that 0/1 exitCode. You can write it from scratch or use electron-quick-start's test-template branch as a starting point, which is the same template used by Electron Fiddle. You can clone it from the command line with git clone --branch test-template https://github.com/electron/electron-quick-start, then use a web browser to upload it to gist.github.com.

After uploading your test to gist.github.com, file an Electron bug report. The bug report template will ask for a "Testcase Gist URL", which is where you'll provide the Gist URL.

By maintainers

Much like trop, you can start BugBot with issue comments. To begin bisection, add a comment that looks like this:

/bugbot test [gist] [platforms...] [versions...]

• If no gist is given, use the issue body's Testcase Gist URL.
• If no platforms are given, test on Linux, macOS, and Windows.
• If no versions are given, test the first version and latest version of each prerelease branch, of each supported branch, and of the two branches before that.

/bugbot bisect [gist] [goodVersion [badVersion]]

• If no gist is given, use the issue body's Testcase Gist URL.
• If no goodVersion is given, use the issue body's Last Known Working Electron Version or an old version.
• If no badVersion is given, use the issue body's Electron Version or the latest release.

Deployment

Environment variables

Name	Module	Description	Default
BUGBOT_BROKER_CERT or BUGBOT_BROKER_CERT_PATH	Required by Broker if BUGBOT_BROKER_URL is https	The data (or the path to it) to use as the cert option to https.createServer().	None
BUGBOT_BROKER_KEY or BUGBOT_BROKER_KEY_PATH	Required by Broker if BUGBOT_BROKER_URL is https	The data (or the path to it) to use as the key option to https.createServer().	None
BUGBOT_BROKER_URL	Required by all	The base URL for the broker, e.g. https://bugbot.electronjs.org:8443.	None
BUGBOT_CHILD_TIMEOUT_MS	Runner	When to cancel a hung child	5 minutes
BUGBOT_FIDDLE_EXEC	Runner	Used to invoke electron-fiddle. This can include other space-delimited command-line arguments, e.g. xvfb-run electron-fiddle	'which electron-fiddle'
BUGBOT_POLL_INTERVAL_MS	Issue Manager, Runner	How frequently to poll the Broker	issue-manager: 500 msec. runner: 20 sec
BUGBOT_AUTH_TOKEN	Required: Issue Manager, Runner; Optional: Broker	The auth token for communications with the Broker
BUGBOT_GITHUB_LOGIN	Issue Manager	The name of the GitHub app registered for the Probot client
BUGBOT_LOG_METRICS_URL	Broker	The remote Loki endpoint to send log metrics to
BUGBOT_LOG_METRICS_AUTH	Broker	The (Basic) auth credentials to authenticate log metrics requests with

Development

BugBot is split into multiple modules, connected in development via Yarn Workspaces and TypeScript Project References. Each module can be found within the modules/ top-level directory.

After cloning BugBot, run the following commands to set up your workspace:

$ yarn
$ yarn run build

Note: The latter command is required due to a caveat with TypeScript Project References and will hopefully be remedied automatically in the future. If you don't run yarn run build then you may see errors in your editor for modules that have not been built (or are not up-to-date and need to be rebuilt).

bugbot's Issues

in-comment commands do not get activated when surrounded by whitespace

This command will trigger a bisect:

/test bisect

But this will not:

/test bisect

add github actions to yarn:test PRs

There aren't any actions right now and we've had a couple of merges that broke tests...

feat: bot must detect issue comments from maintainers

Currently we use a hardcoded array of the bugbot maintainers. It needs to support a list of all the Electron maintainers in order to be useful to the project at large.

If nothing else, the quick-and-dirty fix would be to hardcode the list of Electron maintainers.

I'd suggest we copy this code from trop:

export const isAuthorizedUser = async (context: Context, username: string) => {
  const { data } = await context.github.repos.getCollaboratorPermissionLevel(
    context.repo({
      username,
    }),
  );

  return ['admin', 'write'].includes(data.permission);
};

Since this code is currently in production I don't anticipate any issues with it; but just to be clear in case some issue arises, using this code is not the blocker. The priority is to somehow make bugbot available to all maintainers ASAP whether it's done with long-term code or a short-term hack.

Web interface module

Extending the idea of #52, we should have a more general web interface module for Bugbot:

• A public log file viewer
• Administrative controls for Bugbot maintainers
  • Token control
  • Viewing and manually editing the job list (e.g. for diagnosing a bug or cleaning up)
  • Emergency controls

discussion: test matrix markdown appearance

When the probot client is waiting on a test matrix and gets new information from the broker, it should update its comment with an updated matrix.

This is a smaller piece of the test matrix feature but it smells like a feature that might get some bikeshedding, so I thought I'd preemptively add an issue for it. 🙂

In previous discussions, we agreed that plaintext tables would be both duller and harder to read than something styled, e.g. background colors in table cells. Unfortunately It's not possible to add html style in GitHub issue comments and there are limited options for adding visual cues. One option is to use emojis. Here are some mockups of possible versions:

Mockup 1

	macOS 11	Ubuntu 21.04	Windows 10
Electron 8	✔️ Passed	✔️ Passed	💥 Runner Error
Electron 9	✔️ Passed	✔️ Passed	💥 Runner Error
Electron 10.0.0	✔️ Passed	✔️ Passed	💥 Runner Error
Electron 10.x.y	❌ Failed	❌ Failed	💥 Runner Error
Electron 11.0.0	❌ Failed	❌ Failed	💥 Runner Error
Electron 11.x.y	❌ Failed	⏳ Waiting	💥 Runner Error
Electron 12.0.0	❌ Failed	❌ Failed	💥 Runner Error
Electron 12.x.y	❌ Failed	🐢 Timed Out	❌ Failed
Electron 13 Beta	❌ Failed	❌ Failed	💥 Runner Error

Thoughts: this is the one we worked on in the hackmd so I've listed it first, but after viewing it on GitHub... this feels very busy and also cartoonish. I don't think this one will age very well.

Mockup 2

	macOS 11	Ubuntu 21.04	Windows 10
Electron 8	✔️ Passed	✔️ Passed	🔸 Error
Electron 9	✔️ Passed	✔️ Passed	🔸 Error
Electron 10.0.0	✔️ Passed	✔️ Passed	🔸 Error
Electron 10.x.y	🔻 Failed	🔻 Failed	🔸 Error
Electron 11.0.0	🔻 Failed	🔻 Failed	🔸 Error
Electron 11.x.y	🔻 Failed	⚪ Waiting	🔸 Error
Electron 12.0.0	🔻 Failed	🔻 Failed	🔸 Error
Electron 12.x.y	🔻 Failed	🐢 Timed Out	🔻 Failed
Electron 13 Beta	🔻 Failed	🔻 Failed	🔸 Error

Thoughts: using abstract diamonds / circles is less intuitive but the single-color symbols are less "noisy" than a table full of more complex emoji graphics as in Mockup 1. I think this could be revised but the simple emojis are an improvement. Unfortunately there is a limited palette of them on GitHub.

Mockup 3

	macOS 11	Ubuntu 21.04	Windows 10
Electron 8	🟢 Passed	🟢 Passed	🟠 Error
Electron 9	🟢 Passed	🟢 Passed	🟠 Error
Electron 10.0.0	🟢 Passed	🟢 Passed	🟠 Error
Electron 10.x.y	🔴 Failed	🔴 Failed	🟠 Error
Electron 11.0.0	🔴 Failed	🔴 Failed	🟠 Error
Electron 11.x.y	🔴 Failed	🟡 Pending	🟠 Error
Electron 12.0.0	🔴 Failed	🔴 Failed	🟠 Error
Electron 12.x.y	🔴 Failed	🔵 Timeout	🔴 Failed
Electron 13 Beta	🔴 Failed	🔴 Failed	🟠 Error

This one feels like the winner to me, or at least the best one of these three. Simple, easy to read, and not noisy.

I knock a couple of points off it for being maybe too dull but what it lacks in flair now will be made up for by not driving us nuts the 100th time we read a bugbot table. Maybe the colors could be tweaked, but IMO this one is pretty close.

feat: Runner support for running individual tests

As a sibling function to Runner.runBisect(), called from Runner.poll(). In terms of polling the Broker, works the same as a bisect but pulls in the version property from /api/jobs/$job_id (GET) and uses that when calling electron-fiddle from the command line.

STORY: The Happy Path

Kim works for ZoneApp, a machine learning startup. The app uses your webcam to analyze your face, and track when you are "in the zone". Any time someone is in the zone, notifications to all of their apps are suppressed.

The app runs on Mac, Windows, and Linux.

Kim is testing ZoneApp on the latest version of electron, hoping to upgrade. Unfortunately the application crashes when loading. Kim digs around a little, and is able to develop a repro case.

Kim goes to open an issue on the electron repository. She chooses the regression template, and fills out all of the fields except for the Fiddle Gist URL, because she doesn't know what that is. The issue is created and labeled as regression.

Our friendly neighborhood bot welcomes Kim, and informs her that regressions require a runnable Fiddle test case. The bot links to an explainer page, which contains a short video on getting started with fiddle. there is also a "launch in fiddle" example that weill launch fiddle and load the example.

Kim crafts the example, and edit the body of the issue to include the gist url. The friendly neighborhood bot notices the update, see6 that there is a valid fiddle link, and gets to work. thankfully the gist works great, and our bot is able to bisect the issue to an exact version of electron that failed.

The bot leaves a comment indicating the exact version of electron that has failed, and links to the commits in that version. It also adds the label regression/confirmed.

A wonderful maintainer comes along and creates a PR to fix the issue. Open regressions are run against all new PRs. the pull request fixes the regression, and our bot cross links between the original issue and the PR. The bot adds the label regression/confirmed/fix

The PR is reviewed and merged, and the open regressions are run against master.Assuming the regression test still passes, then a regression/confirmed/fix/merged label is added to Kim's original issue, and it is closed.

Move issue parsing code to `bot` module

Since the issue content is handled client-side, I think it should belong in the probot code.

bugbot should be available 24/7

Our current deployment is focused on the current sprint's goal of proving it can work end-to-end and some of the deployment is done in an ad-hoc manner. After we're past this sprint, bugbot's runners and broker need a more permanent place to live.

update the issue playbook in governance to ask for a test

Currently the playbook asks for a repro testcase forked from electron-quick-start. We should update this to specifically ask for a test so that bugbot will able to aid in triaging more issues.

outreach to releasers

Most of the maintainers know that this project is a work in progress, but since the tool is going to be "opt-in" for awhile it's important that they know how to use it, including but not limited to the people on triage duty in the releases WG.

Once we're past this demo sprint and have bugbot running as a 24/7 feature somewhere, we should help triagers to know what's available and how to use it.

re-enable warnings disabled in .eslintrc + fix the warnings

Discussed in #80.

We've added typescript-eslint/recommended-requiring-type-checking but disabled some of its tests because there were a lot of warnings that were out-of-scope for #80. This issue tracks the remaining warnings.

Modularize broker routing

Currently the bulk of the broker module's web API processing code lives in a single server.ts file. Separating the web API code into discrete pieces (e.g. similar to how our routing splits job routes from token routes) could also provide a better method of testing the web API processing code separately from the Broker itself.

[Bug]: Things Not Work

Preflight Checklist

I have read the Contributing Guidelines for this project.
I agree to follow the Code of Conduct that this project adheres to.
I have searched the issue tracker for a feature request that matches the one I want to file, without success.

Electron Version

11.0.2

What operating system are you using?

Windows

Operating System Version

10

What arch are you using?

x64

Last Known Working Electron Version

10.1.6

Expected Behavior

Testing

Actual Behavior

World

Testcase Gist URL

https://gist.github.com/clavin/59444f92bffd5730944a0de6d85067fd

Consider showing OS version info in bot's issue comments

Currently it says 'Linux', 'macOS', and 'Windows'.

Perhaps it should be more explicit about version numbers, e.g. 'Windows 10' or 'macOS 11.04', and 'Ubuntu 20.04"

Probot Client should be able to update its own comment

Right now the probot client's workflow for bisect is

1. start bisect
2. wait for bisect to finish
3. give the results in a new comment

Instead, it should be:

1. add a comment saying that the bisect is beginning
2. start bisect
3. wait for bisect to finish
4. give the results by updating the comment created in step 1

The bot should be refactored in a way s.t. it should not care whether the first comment exists yet or not, e.g. GithubClient.setBugbotComment(markdown: string) (or whatever function name) so that the comment is created if it doesn't exist, or updated if it does exist.

We will need this functionality for the matrix as well, but adding it for the existing bisect feature means it could be developed now before the matrix code is ready.

Consolidate broker api client implementations into one module

Currently, there are approximately three different implementations (or parts of impls) of clients to communicate with the broker:

• The GitHub bot module
• The runner module
• The broker's own tests

It would be better to consolidate these into their own @electron/bugbot-broker-client module that has its own unit tests and integration tests with the broker server module.

probot client testing is poor

Right now the probot client's testing coverage is ~35%.

It's difficult to make changes that effect the probot client + other modules (or just the probot client itself) because it's difficult to test those changes.

refactor: make env vars consistent + prefixed

• right now we have two different names for the broker url prefix between runner & bot. This should just be one variable.
• likewise, the broker uses a _PORT env var but it could sniff the protocol & port from that same base_url variable and get rid of the rest
• all env vars should be given a name prefix e.g. BUGBOT_
• we should document the env vars in the top-level README
• the modules/shared/ code errors out if it can't find a required variable, and logs the error to debug(). It should probably also log to console.log or console.error in case someone is running without DEBUG turned on :D

bug: some version + test combos block runner with Uncaught Exception popup dialog

Sometimes a test will error out and a confirmation dialog pops up:

...and since Runner is headless, there's no way to click 'OK' to continue on with life. This dialog is generated by this code in Electron that pops it up iff there's an uncaught exception.

Possible diagnosis: maybe the answer is for Fiddle's runner to listen for process' uncaughtException event iff Fiddle is running in headless mode; but since I don't have time to triage it this minute and am not positive it's a Fiddle issue, I'm filing it here in bugbot's tracker until I can spend more time on triage.

implement the test 'matrix'

Discussed in the responses / stories document.

This is the $version x $platform matrix table showing the results of running a test on those configurations.

Subtasks:

• #96
• #100
• #97
• #104
• #98
• #110
• #102

should issue-parser be in bot/ instead of shared/?

It doesn't seem that it would be used in the broker or runner, so I'm not sure I understand why it's in the shared submodule?

integration tests can't test runner failures

right now the electron-fiddle fake that we have in integration-tests/tests/fixtures/electron-fiddle does a good job of parroting back the right log messages for a given session, but it doesn't parrot the exit code. That's OK in our current integration tests because we're only testing a success path, but it won't work as soon as we test a failure path 🙂

feat: add `/bugbot test` to probot client

Some of the groundwork has already been done:

• #100 added prerequisite code for parsing /bugbot test commands. Currently doesn't do anything with them. (merged)
• #96 adds prerequisite code to figure out what Electron versions to use in the matrix. (ready for review)
• #97 will add prerequisite code for overwriting an issue comment so we can update the matrix when new information comes in.

Now we need code to tie all these pieces together.

• GithubClient needs to kick of N test jobs, maintain state for >1 job per issue, keep polling until done, and periodically update the matrix with the latest data.

store state between sessions

IIUC heroku dynos get restarted once a day. We should persist state in the broker (and possibly in the probot client too?) to safeguard against resets, e.g. some simple store like mongoose or redis.

hello

helloiojoi

coverage tests don't include cross-monorepo testing

For example, shared/env-vars.ts doesn't appear in our tests even though it's used in the bot, broker, and runner.

Likewise, Task.getRawLog() is showing as not covered even though it's exercised in the integration tests.

My guess is what's happening is that code outside of the current workspace is being loaded as .js and is discarded by our jest exclusion rules. I think we'd get more accurate coverage reports if jest included out-of-workspace code as ts instead.

feat: log bugbot usage

As discussed in the Jul 1 meeting.

We want to be able to track how useful bugbot is to the project, and one meaningful proxy for that is how much it gets used.

Probably this should be done at the broker level by adding some persistent storage e.g. a database and logging information about issue / gist / login / os.platform / result

[Bug]: new bug

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for a feature request that matches the one I want to file, without success.

Electron Version

12.0.0

What operating system are you using?

Ubuntu

Operating System Version

buntu

What arch are you using?

x64

Last Known Working Electron version

No response

Expected Behavior

broke

Actual Behavior

broke n

Testcase Gist URL

https://gist.github.com/this/is/the/gist/url

Add authorization to Broker

The broker should require clients (runners, bots) to provide the standard Authorization header used in most web APIs so only clients that have been granted secret tokens can perform actions on the broker. Simple security stuff to prevent unknown users from vandalizing the system.

Add integration tests between bot and broker

feat: ability to pass manual token to auth layer

issue comment commands should offer more options

Currently we have /run bisect which works but only works in one specific way, e.g. you need to know exactly how to format an issue template to provide the good & bad versions for bisection. If you miss any part, nothing will happen.

Also, we might want to make it more explicit that bugbot is being invoked, in the same way we have /trop ${action} ${args...} maybe we should support options in the command.

• /bugbot bisect $gist $good $bad
• /bugbot test $gist [$version]

idea: store logs as static files?

Issue comments are going to leave breadcrumbs linking to bugbot job logs, so they're going to need to be persistent. One relatively low-tech way of doing this would be to store these as actual html files keyed by the job id.

Related: #41.

`@typescript-eslint/parser` config errors for tests

Opening a test file after #80 yields the following error in VSCode:

Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: modules/bot/test/github-client.spec.ts.
The file must be included in at least one of the projects provided

Linting then doesn't work on these files at all.
When the tsconfig.json is changed to include /test/**/*, the build command then fails to compile (ref #89).

better detection of whether the next action is for the bug reporter or for a maintainer

In the responses/stories document we outlined a lot of potential error cases & edge cases, none of which are currently being handled. We should add these. The most important one is probably to identify who should do triage on a bisect failure, e.g. determine if it's a runner error or a bug in the test.

feat: add a link to the logfile in the bugbot comment

Also it should probably link as soon as the job is added, rather than when the runner completes. That way the user can watch the progress as things go.

when a new issue is reported & has a test, automatically test it

When a new issue is reported, even if it's not a regression (e.g. even if no "last known working version" was filled out), bugbot should see if it can confirm the issue and report back in a comment + labels.

The full implementation is still up for debate but some pieces are clear:

• Update labels as necessary so that bugbot can correctly handle future updates to the issue
• Make clear whose responsibility the next step is: a maintainer or the OP?
• Give that person the info they need to make the next step

All cases:

1. MUST set a label reflecting the bugbot state of the bug. Bugbot needs this to correctly determine the issue's state the next time there is an update in the issue.
2. MUST report back in the bugbot issue comment the test results. Related Fiddle discussion @ electron/fiddle#672

if the test failed

1. (brainstorming) MAY set other labels e.g. a 'confirmed'
2. (brainstorming) MAY set platform labels depending on which or all platforms the test passed or failed on
3. (brainstorming) MAY set version numbers depending on branches affected by the bug

If the test passed:

2. (brainstorming) MAY notify OP via an @ in the comment
3. (brainstorming) MAY mark the issue as blocked pending a gist revision

If the test failed to run:

2. (brainstorming) MAY notify a maintainer @ in comments
3. (brainstorming) MAY notify a maintainer in Slack
4. (brainstorming) MAY include collapsed text of the fiddle output in bugbot's issue comment
5. (brainstorming) MAY have a link to a cloud CI output of the failed run

[Bug]: this be a b ug

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for a feature request that matches the one I want to file, without success.

Electron Version

12.0.0

What operating system are you using?

Ubuntu

Operating System Version

Ubuntu 20.10

What arch are you using?

x64

Last Known Working Electron version

11.0.0

Expected Behavior

broke

Actual Behavior

broke n

Testcase Gist URL

https://gist.github.com/this/is/the/gist/url

feat: broker must accept posted Test jobs

Right now it only accepts bisect jobs from the probot client.

Also, data validation on those posted jobs is a mess.

improve metrics

for the July 22nd sprint we're just tracking # of jobs created. But there's general consensus on improving this, e.g. distinguishing test success / test failure / system failure and maybe logging stats each time a job's history is patched.

Bundle modules for deployment

Modules should be bundled up and tree-shaken and just generally optimized for deployments using a tool like rollup.js. This helps keep our deployments slim in many ways including ignoring other modules that will not run, i.e. the broker code doesn't need to be present on a runner deployment.

potential security vulnerability: runner should sanitize env before spawning electron-fiddle

We pass a lot of bugbot's config into the web services via environment variables, including potentially in the future secrets/tokens. When a runner calls spawn() to start electron-fiddle, we should sanitize the environment variables so that none of that is visible to user-supplied test gists.

broker needs to intervene if a runner times out

Currently, if a runner disappears in the middle of a job, that job will just hang indefinitely.

The broker should notice when a runner isn't communicating. It could definitely amend the log, and it perhaps should also mark the job as unclaimed so that another runner can try.

[Bug]: @@@@

Preflight Checklist

I have read the Contributing Guidelines for this project.
I agree to follow the Code of Conduct that this project adheres to.
I have searched the issue tracker for a feature request that matches the one I want to file, without success.

Electron Version

12.0.0

What operating system are you using?

Windows

Operating System Version

10

What arch are you using?

x64

Last Known Working Electron version

11.0.0

Expected Behavior

Testing

Actual Behavior

World WWWWWWWWW

Testcase Gist URL

https://gist.github.com/erickzhao/24848aefcbb922444b148321a1821be6

feat: persistent logging for the broker

We currently keep the broker's log in memory, but that's not ideal -- it means the logs will disappear forever if the broker gets restarted.

Whatever persistence solution we use needs to play nice with Heroku, since that's where the broker is deployed.

Previous discussions have been redis or mongodb. Whoever claims this issue gets to decide and do it the way they like!

This is related to (and possibly a duplicate of) #52

when a new issue is reported, add labels based on template values

This could save triagers a little time by auto-triaging some of the issue.

• if a platform is specified, set the corresponding platform label
• if title matches [Bug]:, add the bug label
• if a parseable version number was specified, set the corresponding N-x-y label

feat: ability to trigger work via commands in comments

Suggested by @clavin here; giving it an issue so that the idea doesn't get lost

Maybe I'm thinking too far head, but question: should this bot be proactive in identifying things it can run in an issue, or should it instead be invoked like a command? The benefit of the latter is that we can easily activate the bot after some back-and-forth, e.g. asking the reporter to create a fiddle.

This could be useful when we need to kick off Fiddle runs manually, the same way we do with Trop when it gets stuck.

chore: add app.json for the buildpacks

As discussed @ #32 (review).

This wasn't a blocker for the Thursday demo and we're deferring anything that's not a blocker, so opening a new issue so that we can get this in the next sprint.

CC @MarshallOfSound @erickzhao

Log file viewer generation

Currently our log file viewer just spits out the raw log with some fancy html wrapped around it, making our viewer both fragile (seemingly-innocuous output could accidentally break it) as well as insecure (textbook XSS). We should design a viewer that is more secure and resilient while staying ergonomic to develop.