rjz / supertest-session

Persistent sessions for supertest
Other
88 stars 21 forks source link

Allow app to be instance of https.Server for address parsing #34

Closed aphix closed 6 years ago

aphix commented 6 years ago

I ran into an issue attempting to test an app via https.createServer(opts, app); this pull request will allow app to be an instance of https.Server (instead of only http.Server) when parsing the server URL.

When testing against HTTPS with a self-signed certificate a root CA will be needed, but this option can be provided to supertest-session via:

var app = express()

// ...setup express app... 

var httpsApp = require('https').createServer({
    key: fs.readFileSync('../path/to/host-key.pem'),
    cert: fs.readFileSync('../path/to/host-cert-with-chain-from-ca.crt'),
    rejectUnauthorized: true,
}, app);

var testHttpsSession = require('supertest-session')(httpsApp, {
    ca: fs.readFileSync('../path/to/ca-cert.crt')
});

As for adding tests, I didn't feel it was worth bringing fake certificate files into this repo for the sake of testing https, but have no problem adding them if they're required to complete the merge.

I have been using this fork locally in a number of projects (via a private npm repo) and it doesn't appear to have any negative effect, or change in functionality.

Lastly, I was unsure if a package.json change was required, but didn't want it to be confused with the existing version.

Let me know if there is anything I can do to help make this easier to merge.

rjz commented 6 years ago

Hey, @aphix! This makes good sense to me—probably overdue, to be honest. If you'd be willing to drop the package.json change, I'll merge this and bump the version.

aphix commented 6 years ago

Sure thing! I've pulled the package.json back to pre-fork version number (sorry about that, I'm not familiar with standard PR protocol for version numbers). Hopefully it's alright to have the change as a second commit; if you need me to make a new branch without the package.json change, I'd be happy to do so.

Let me know if there's anything else you need me to do for a merge.

Fantastic module, by the way =)

Thanks!

rjz commented 6 years ago

Thanks, @aphix! Merged and published as v3.2.0.