Skip to content

Add better error messages for partial failing variant analysis#1295

Merged
aeisenberg merged 3 commits intomainfrom
aeisenberg/result-log
Apr 20, 2022
Merged

Add better error messages for partial failing variant analysis#1295
aeisenberg merged 3 commits intomainfrom
aeisenberg/result-log

Conversation

@aeisenberg
Copy link
Copy Markdown
Contributor

Two scenarios handled:

  1. no database for existing repo
  2. repo does not exits (or no access rights for current user)

In either case, an error message is sent to the logs, with a notificaiton
in a popup.

Log message looks like this:

Successfully scheduled runs. See https://github.com/dsp-testing/qc-run2/actions/runs/2164299232.
Some repositories could not be scheduled.
Invalid repositories: aeisenberg/xxx, other/idontexist
Repositories without databases: aeisenberg/actions-test, aeisenberg/actions-test2

Two scenarios handled:

1. no database for existing repo
2. repo does not exits (or no access rights for current user)

In either case, an error message is sent to the logs, with a notificaiton
in a popup.
@aeisenberg aeisenberg requested review from a team as code owners April 13, 2022 23:33
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks! Nice to tell the user when we skip repositories 😎

A few comments (and some failing PR checks) 📝

Comment thread extensions/ql-vscode/src/remote-queries/run-remote-query.ts Outdated
Comment thread extensions/ql-vscode/src/remote-queries/run-remote-query.ts Outdated
Also, change text slightly.
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

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

Thanks! Just some tiny optional comments 🥇

import { expect } from 'chai';
import { parseResponse } from '../../../remote-queries/run-remote-query';

describe('run-remote-queries', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the test 🙌🏽
Very minor - could we change the name of this test (and the file name itself) to run-remote-query? Just for consistency with the name of the file we're testing 😸

it('should parse a response with invalid repos and repos w/o databases', () => {
const result = parseResponse('org', 'name', {
workflow_run_id: 123,
repositories_queried: ['a/b', 'c/d'],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we also want to test the case where no repositories succeeded?

And rename test file.
@aeisenberg
Copy link
Copy Markdown
Contributor Author

Comments addressed. Thanks for the review!

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