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

chore: add http2 alpn test using fastify #34

Merged
merged 5 commits into from
May 30, 2023

Conversation

mkaufmaner
Copy link

No description provided.

@mkaufmaner mkaufmaner mentioned this pull request May 25, 2023
20 tasks
@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.68 🎉

Comparison is base (2257b16) 85.05% compared to head (eceb7a7) 85.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##            http2      #34      +/-   ##
==========================================
+ Coverage   85.05%   85.74%   +0.68%     
==========================================
  Files          75       76       +1     
  Lines        6719     6720       +1     
==========================================
+ Hits         5715     5762      +47     
+ Misses       1004      958      -46     

see 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@metcoder95
Copy link
Owner

@KhafraDev, is better to have a custom implementation instead of adding other dev dependencies for testing out the ALPN negotiation?

If so, I haven't tried yet but we should work on a small fixture that supports ALPN negotiation for testing.


const { Client } = require('..')

test('Should support secure HTTPS connection', async (t) => {
Copy link
Owner

Choose a reason for hiding this comment

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

This scenario is not really needed, we are just looking to test the ALPN negotiation

Copy link
Author

Choose a reason for hiding this comment

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

Yep, saw your comment in the other PR and will work towards using native http/2.

Copy link
Author

Choose a reason for hiding this comment

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

Looks like I can use the http2 compatibility layer for this. Will try and modify accordingly.


const response = await client.request({
path: '/',
method: 'GET'
Copy link
Owner

Choose a reason for hiding this comment

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

It's better if we try a using a request that can include a body (PATCH/POST) so we can really test everything is transmitted as expected on both ways

@KhafraDev
Copy link

I'm sorry, I completely misunderstood what was being asked. Adding dev dependencies is fine.

@mkaufmaner
Copy link
Author

I'm sorry, I completely misunderstood what was being asked. Adding dev dependencies is fine.

Haha, well I think I have already pretty much got it working without fastify anyways. Will update shortly.

@mkaufmaner
Copy link
Author

mkaufmaner commented May 26, 2023

@KhafraDev I removed fastify as a dev dependency and I still somewhat agree that external dependencies shouldn't be added unless absolutely necessary. Even if the external dependency is a dev dependency. I ended up getting ALPN to work with barebones http2.createSecureServer and the allowHTTP1: true option.

@metacoder I added a test for handling request bodies as well using a POST method.

@metcoder95 This is ready to merge when you are.

package.json Outdated Show resolved Hide resolved
@mkaufmaner
Copy link
Author

@metcoder95 I believe this is ready to be merged.

@metcoder95 metcoder95 merged commit d32a43d into metcoder95:http2 May 30, 2023
metcoder95 pushed a commit that referenced this pull request Jun 21, 2023
* chore: add http2 alpn test using fastify

* chore: update to test https 1 with http2

* chore: update alpn test to return server request alpn protocol and http version

* chore: add alpn with body

* fix: remove fastify from package json
metcoder95 pushed a commit that referenced this pull request Jul 21, 2023
* chore: add http2 alpn test using fastify

* chore: update to test https 1 with http2

* chore: update alpn test to return server request alpn protocol and http version

* chore: add alpn with body

* fix: remove fastify from package json
metcoder95 pushed a commit that referenced this pull request Aug 20, 2023
* chore: add http2 alpn test using fastify

* chore: update to test https 1 with http2

* chore: update alpn test to return server request alpn protocol and http version

* chore: add alpn with body

* fix: remove fastify from package json
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.

3 participants