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

fix: update Koa2 plugin to await next #339

Merged

Conversation

DominicKramer
Copy link
Contributor

PR #336 fixed the Koa plugin, but didn't fix the Koa2 plugin.

Fixes: #335

PR googleapis#336 fixed the Koa plugin, but didn't fix the Koa2 plugin.

Fixes: googleapis#335
@DominicKramer DominicKramer requested a review from a team April 11, 2019 17:29
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 11, 2019
@DominicKramer
Copy link
Contributor Author

I have manually verified that errors are now correctly reported with this fix in Koa2. In particular, I used the following example code:

import * as Koa from 'koa';
import {ErrorReporting} from '@google-cloud/error-reporting';

const errors = new ErrorReporting({
  projectId: 'my-project',
  reportMode: 'always'
});

const app = new Koa();

app.use(errors.koa2);

app.use(async ctx => {
  ctx.throw(new Error('This is an error from Koa2'));
});

app.use(async ctx => {
  ctx.body = 'Hello World';
});

app.listen(3000);

Issue #337 will create samples tests to automatically verify our sample code work as expected.

@codecov
Copy link

codecov bot commented Apr 11, 2019

Codecov Report

Merging #339 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #339   +/-   ##
======================================
  Coverage    96.1%   96.1%           
======================================
  Files          34      34           
  Lines        1694    1694           
  Branches       76      76           
======================================
  Hits         1628    1628           
  Misses         49      49           
  Partials       17      17

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db8136d...f27d602. Read the comment docs.

@DominicKramer DominicKramer merged commit cb6f9ac into googleapis:master Apr 11, 2019
@bcoe
Copy link
Contributor

bcoe commented Apr 11, 2019

@kjin @DominicKramer would this have been a pain to catch in test? might be worth adding a ticket just to put some testing around this case.

@kjin
Copy link
Contributor

kjin commented Apr 11, 2019

It was mentioned above that #337 is a possible way to test behavior using samples, but it's true that it should be tested as a unit test. @DominicKramer does that sound good?

@DominicKramer
Copy link
Contributor Author

@kjin @bcoe I have an issue for tracking the adding of tests at issue #337. I think samples tests would be the best approach to take because we want to verify that the plugin can be used.

Testing every single aspect of the plugin separately I think would be not be worth the effort when testing how the plugin overall is used could be done. This is because, to be complete, we would need to write tests for every version of every plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

koa2 interface not awaiting next middleware
4 participants