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

integration tests #415

Merged
merged 7 commits into from
Nov 2, 2023
Merged

integration tests #415

merged 7 commits into from
Nov 2, 2023

Conversation

mtuchi
Copy link
Collaborator

@mtuchi mtuchi commented Oct 19, 2023

Summary

Add integration tests. These tests are using nodejs http and https modules for creating a server that is used in tests.
The http server is used for testing redirects, while the https server is used for testing the tls support.
In addition the https server is also using koa and koa-sslify packages.

You will notice a test/helpers/ directory which contain certs.js. These are the certificates that we are using for the https server for testing

Ref #402 #406

Review Checklist

Before merging, the reviewer should check the following items:

  • Does the PR do what it claims to do?
  • Are there any unit tests? Should there be?
  • Is there a changeset associated with this PR? Should there be? Note that
    dev only changes don't need a changeset.

@mtuchi mtuchi changed the title 402 integration tests integration tests Oct 19, 2023
@mtuchi mtuchi marked this pull request as ready for review October 19, 2023 13:47
Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

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

I'm afraid I'm seeing a number of high-level issues here, I'm not quite sure what's happened.

The test/intergration.js file needs renaming to test/integration.js (there's only one R in integration). This is on the base branch and affects many of our branches, but let's get it sorted out here and merge it down.

I didn't think you wanted to use the koa server, but at the moment the tests seem to rely on it? Also at the moment we're declaring one set of servers in intergration.js, but also importing a different set of servers in test/helpers/server.js. So I'm a little confused about the intention. This is the sort of thing where a little comment in the PR description can help dispel confusion and make reviewing a little easier.

There is too much logging in the test code. Logging is useful while writing tests but it's really annoying in CI or when doing other work because the test output is spammed with meaningless stuff. So I'd really like to tighten up the use of logging here.

Also, all integration tests are failing locally. CI is failing too.

@mtuchi
Copy link
Collaborator Author

mtuchi commented Oct 30, 2023

@josephjclark I don't know why the CI is failing because locally all test are passing

it turns out in ubuntu any port < 1024 needs root permission
@mtuchi
Copy link
Collaborator Author

mtuchi commented Nov 1, 2023

It turns out in ubuntu any port < 1024 needs root permission, So i have updated the httpsPort to 1443 cc @josephjclark

@mtuchi mtuchi requested a review from josephjclark November 1, 2023 07:50
@josephjclark
Copy link
Collaborator

@mtuchi Great catch on the port number, I had no idea!

What about koa and http - do we need both server implementations?

remove koa implementation
@mtuchi
Copy link
Collaborator Author

mtuchi commented Nov 2, 2023

What about koa and http - do we need both server implementations?

Hiya @josephjclark i have removed the koa implementation, and use the nodejs https module

Copy link
Collaborator

@josephjclark josephjclark left a comment

Choose a reason for hiding this comment

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

Merci!

@josephjclark josephjclark merged commit be82842 into 316-http-rewrite Nov 2, 2023
@josephjclark josephjclark deleted the 402-integration-tests branch November 2, 2023 09:21
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