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

Mocha Testing changes #48

Merged
merged 10 commits into from Jan 31, 2017

Conversation

Projects
None yet
4 participants
@tejashah88
Copy link
Member

commented Jan 31, 2017

  • Removed deprecated dependency 'supertest-as-promised'
  • Maintaining testing format consistency
  • Added tests to verify that server does not always bind to both HTTP and HTTPS ports (#46)
  • Added more node versions to run mocha tests on Travis-CI
Mocha Testing changes
* Removed deprecated dependency 'supertest-as-promised'
* Maintaining testing format consistency
* Added tests to verify that server does not always bind to both HTTP and HTTPS ports (#46)

@tejashah88 tejashah88 requested a review from dblock Jan 31, 2017

tejashah88 added some commits Jan 31, 2017

expect(err).to.not.exist;
expect(result).to.be.true;

isPortTaken({ port: 3000 }, function(err, result) {

This comment has been minimized.

Copy link
@dblock

dblock Jan 31, 2017

Collaborator

This is identical to the above, bad copy-paste?

});
});

it("should have the HTTP and HTTPS server instances running", function() {

This comment has been minimized.

Copy link
@dblock

dblock Jan 31, 2017

Collaborator

This is the problem, it seems wrong to me. When I enable HTTPs, why should I have both HTTP and HTTPs? No other server works like this. There should be only 1 port option and it should be HTTP or HTTPs.

This comment has been minimized.

Copy link
@mreinstein

mreinstein Jan 31, 2017

Contributor

When I enable HTTPs, why should I have both HTTP and HTTPs?
No other server works like this

one use case for having both: you want to have all traffic sent over http redirected to https.
modern sites should never run http, unless they need to for some compatability reasons.

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Jan 31, 2017

Author Member

Maybe a flag explicitly stating that HTTP is enabled or not would make sense. For example, if I wanted only HTTPS to be enabled, it could be the following:

{
  httpEnabled: false,
  httpsEnabled: true
  ...
}

This comment has been minimized.

Copy link
@mreinstein

mreinstein Jan 31, 2017

Contributor

another option is just ditch https in alexa-app-server. In production most people don't run ssl directly with node; there is better software to do this. (load balancers, nginx reverse proxy, etc.)

It also makes local development easier, because there's no certificate ceremony to deal with.

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Jan 31, 2017

Author Member

Interesting option. If we do decide to go that route, we should at least add some tutorials for setting up with HTTPS if people do want to do so.

Speaking on the same note, it might be a great idea to have a wiki pages so that all relevant tutorials and API documentation can stay there and the README is not as long.

This comment has been minimized.

Copy link
@dblock

dblock Jan 31, 2017

Collaborator

I think ditching HTTPs when it's implemented and working seems unnecessary. I am against two flags, the default it HTTP and if you want, you get HTTPs. There should be no fallback if SSL setup fails the server shouldn't start.

No modern server, Apache, IIS, Tomcat, Express, ..., listens on two ports AFAIK. I could be wrong, but If you want to do http redirecting to https you would make separate instances, implement something like HSTS on the first one, etc.

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Jan 31, 2017

Author Member

Another option could be to change the flag name so that it's obvious that only HTTPS is enabled (i.e. something like httpsOnly). Or the flag could be like an enumeration with multiple states, such as the following:

var httpsOptions = {
  DISABLED, REDIRECTED_TO, STRICT_ENABLED //something like that
}

This comment has been minimized.

Copy link
@dblock

dblock Jan 31, 2017

Collaborator

Two minutes ago we were discussing dropping HTTPs altogether ;)

Seems to me that we're trying to do too much. I think there should just be 1 option: https: true, and whatever cert options. This would mimic what every web server out there I know does.

Either way, do you have a preference to what to do @tejashah88? At the very least we should fix the bug that's here of binding to two different ports, that's totally unexpected.

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Jan 31, 2017

Author Member

Well, we need all the ideas that we can get. ;) I do agree on fixing the bug, although as I've said, it should be obvious to the developer. If we were to go with this route, some other changes may need to be employed.

  1. The httpsPort is not needed since that should be handled by the port flag.
  2. The httpsEnabled flag should be either documented in that only https will be enabled, and/or the flag name itself should be changed.

Personally, I don't mind any particular fix, so long as it makes using the library a bit easier in some aspect. @mreinstein thoughts on this?

This comment has been minimized.

Copy link
@dblock

dblock Jan 31, 2017

Collaborator

I like what you're saying @tejashah88, @mreinstein would be nice to hear from you to help clear up any disagreement on what this code should do, it's easier to "vote" with 3 people :)

@coveralls

This comment has been minimized.

Copy link

commented Jan 31, 2017

Coverage Status

Coverage remained the same at 96.023% when pulling 70edacd on tejashah88:master into 9e1b2a9 on alexa-js:master.

expect(err).to.not.exist;
expect(result).to.be.true;

isPortTaken({ port: 3000, host: '127.0.0.1' }, function(err, result) {

This comment has been minimized.

Copy link
@dblock

dblock Jan 31, 2017

Collaborator

This is duplicating the block above unnecessarily. One test is enough :)

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Jan 31, 2017

Author Member

I added that block so that it would test if the server was binding to a specific address.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

I am down with merging this and dealing with #46 separately, @tejashah88 LMK how you feel about it.

You might want to squash these commits, it's a good exercise ;)

@mreinstein

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2017

I have to say I'm a little bit frustrated right now. While more polished testing infrastructure is nice and appreciated, there are serious problems with this module:

#10

#21

#52

Granted one of them is only a few minutes old, but several are months old. These strike me as "house on fire" kinds of issues. These need to be addressed. If alexa-app-server is not able to host a skill that works in alexa skill store, that should be priority.

I'm happy to help with this but we have to prioritize broken shit over nice polish.

@tejashah88

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

@dblock I agree, let's get this PR merged first, then we can talk about it in a new issue.

Normally, I don't squash the commits until I know the PR is ready to be merged. Thanks for reminding me though.

@tejashah88

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

@mreinstein I do agree, and I've been planning to implement #21 and #35 for some time. For now, we can leave the HTTPS handling for another time.

@dblock dblock merged commit c92fbc3 into alexa-js:master Jan 31, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 96.023%
Details
@dblock

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

I merged this. Tests weren't a thing here until recently, so fixing broken stuff without tests breaks other things, so tests are important. I'll take a stab at the https stuff sometime in the future unless someone beats me to it.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

LMK how I can help with the on fire issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.