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

Make the tests work again #1136

Merged
merged 33 commits into from Jan 18, 2016

Conversation

Projects
None yet
4 participants
@profOnno

profOnno commented Dec 14, 2015

This will also work with node v4. But then zombie has to be version 4. Travis-ci won't work (yet) with node 4. (building native extensions requires C++11-compatible compiler), maybe with the newer docker travis-ci.

@cwebber

This comment has been minimized.

Show comment
Hide comment
@cwebber

cwebber Dec 14, 2015

Contributor

Wow, incredible! This looks good to merge to me. I'm going to verify on IRC that nobody wants to jump on a more informed review than my quick scan and then merge it in.

Contributor

cwebber commented Dec 14, 2015

Wow, incredible! This looks good to merge to me. I'm going to verify on IRC that nobody wants to jump on a more informed review than my quick scan and then merge it in.

@profOnno

This comment has been minimized.

Show comment
Hide comment
@profOnno

profOnno Dec 14, 2015

Check the package.json
I did make a update for databank-connect (uses redis-connect as example).
It is pulled straight from my github.

I did forget to use my simplesmtp from github (no tar.gz no the source like i did with databank-connect (i'm learning on the way)). The simplesmtp is deprecated on npm, but I don't see a problem with using it, it's only used in the test code and not on production.

edit:
It seems the change in package.json for the simplesmtp should/can be reversed if this is merged.

profOnno commented Dec 14, 2015

Check the package.json
I did make a update for databank-connect (uses redis-connect as example).
It is pulled straight from my github.

I did forget to use my simplesmtp from github (no tar.gz no the source like i did with databank-connect (i'm learning on the way)). The simplesmtp is deprecated on npm, but I don't see a problem with using it, it's only used in the test code and not on production.

edit:
It seems the change in package.json for the simplesmtp should/can be reversed if this is merged.

@cwebber

This comment has been minimized.

Show comment
Hide comment
@cwebber

cwebber Dec 15, 2015

Contributor

@profOnno Okay, I don't know anything about the databank-connect stuff. Could you ping @evanp about it? I'm happy to merge this otherwise.

Contributor

cwebber commented Dec 15, 2015

@profOnno Okay, I don't know anything about the databank-connect stuff. Could you ping @evanp about it? I'm happy to merge this otherwise.

@harrisbinkhurram

This comment has been minimized.

Show comment
Hide comment
@harrisbinkhurram

harrisbinkhurram Dec 15, 2015

hey the new pump.io doesn't seem to be working on ubuntu, after installing dependencies im stuck with use of const in strict mode

any leads?

harrisbinkhurram commented Dec 15, 2015

hey the new pump.io doesn't seem to be working on ubuntu, after installing dependencies im stuck with use of const in strict mode

any leads?

@profOnno

This comment has been minimized.

Show comment
Hide comment
@profOnno

profOnno Dec 15, 2015

@harrisbinkhurram: It will be 'fixed' in this pull request. You can get the patch here https://github.com/profOnno/pump.io/tree/r4p/patch. Only connect.patch (and connect_update.sh) matters. The connect_update.sh will apply the patch and rebuild connect without the latest qs.

profOnno commented Dec 15, 2015

@harrisbinkhurram: It will be 'fixed' in this pull request. You can get the patch here https://github.com/profOnno/pump.io/tree/r4p/patch. Only connect.patch (and connect_update.sh) matters. The connect_update.sh will apply the patch and rebuild connect without the latest qs.

Menno Vossen added some commits Dec 15, 2015

Menno Vossen
Menno Vossen
updated versions for: undersocre-contrib, node-uuid, optimist, bunyan…
…, emailjs, mkdirp, dialback-client and gm
Menno Vossen
Menno Vossen
Menno Vossen
Show outdated Hide outdated routes/api.js
Show outdated Hide outdated routes/api.js
Show outdated Hide outdated routes/api.js
Show outdated Hide outdated routes/api.js
Show outdated Hide outdated routes/api.js
Show outdated Hide outdated lib/app.js
Show outdated Hide outdated lib/app.js
Show outdated Hide outdated lib/app.js
cb(err, likes);
});
}
);

This comment has been minimized.

@strugee

strugee Dec 22, 2015

Member

Something's wonky here. Do you perhaps have an extra }?

@strugee

strugee Dec 22, 2015

Member

Something's wonky here. Do you perhaps have an extra }?

This comment has been minimized.

@strugee

strugee Dec 22, 2015

Member

Nope, nevermind. I thought that was a curly bracket.

Anyway, your ); shouldn't be indented so far.

@strugee

strugee Dec 22, 2015

Member

Nope, nevermind. I thought that was a curly bracket.

Anyway, your ); shouldn't be indented so far.

@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Dec 22, 2015

Member

@profOnno this looks really, really good! The vast majority of my comments deal with whitespace and stuff like that, and are extremely trivial to fix. Great work!

That being said, shipping a patch doesn't seem like the best approach. Instead, can you send a PR to upstream connect-databank? Then it'll be fixed for everybody and the Pump.io usage won't be so hacky.

Member

strugee commented Dec 22, 2015

@profOnno this looks really, really good! The vast majority of my comments deal with whitespace and stuff like that, and are extremely trivial to fix. Great work!

That being said, shipping a patch doesn't seem like the best approach. Instead, can you send a PR to upstream connect-databank? Then it'll be fixed for everybody and the Pump.io usage won't be so hacky.

@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Dec 22, 2015

Member

I'm also uncomfortable having the package.json refer to GitHub forks. Can we solve stuff like that some other way, e.g. by upgrading the relevant libraries?

Member

strugee commented Dec 22, 2015

I'm also uncomfortable having the package.json refer to GitHub forks. Can we solve stuff like that some other way, e.g. by upgrading the relevant libraries?

@profOnno

This comment has been minimized.

Show comment
Hide comment
@profOnno

profOnno Dec 22, 2015

Thanks strugee,

I know the patch-thing is not the best thing to do. It's a kind of a place holder. If the tests work, developers can make other adjustments and see the test break or not break depending on their code. If I'm not mistaking Joyent uses it for this kind of problems. The patch for the connect qs story, will resolve itself when changes are in place to upgrade connect to version 2.x if I'm not mistaken (I've seen the package.json qs version capped).

I can/will put a pull request in for the connect-databank. It is a big rewrite combining the connect-redis (mentioned in the connect documentation as example) and the 'old' connect-databank. I can go back a little and just use the patch small change for this PR (I know this is not favoured, but can be used for place holder). The patch will add the stop method to stop the timers on closing the app, needed for vows to end when done. And keep the connect-databank upgrade for a next round.

For the whitespace comment stuff, i looked at jscs.js (http://jscs.info). So the style can be forced automatic. Helps me to be more style-full and helps you writing less style related comments :p. Please give me your opinion on this (see #1137). Should we have Evans coding style along with the other pre-sets? (Jquery, Crockford, Airbnb, Google, Grunt, Idiomatic, etc..) or should we conform to one of the presets. If you want respond, issue #1137. Nice vid from Douglas Crockford about this https://www.youtube.com/watch?v=_EANG8ZZbRs .

I'll fix the style comments asap, and replace this pull request with a new 'cleaned up' one (with squashed commits, I just figured out a way to do that).

profOnno commented Dec 22, 2015

Thanks strugee,

I know the patch-thing is not the best thing to do. It's a kind of a place holder. If the tests work, developers can make other adjustments and see the test break or not break depending on their code. If I'm not mistaking Joyent uses it for this kind of problems. The patch for the connect qs story, will resolve itself when changes are in place to upgrade connect to version 2.x if I'm not mistaken (I've seen the package.json qs version capped).

I can/will put a pull request in for the connect-databank. It is a big rewrite combining the connect-redis (mentioned in the connect documentation as example) and the 'old' connect-databank. I can go back a little and just use the patch small change for this PR (I know this is not favoured, but can be used for place holder). The patch will add the stop method to stop the timers on closing the app, needed for vows to end when done. And keep the connect-databank upgrade for a next round.

For the whitespace comment stuff, i looked at jscs.js (http://jscs.info). So the style can be forced automatic. Helps me to be more style-full and helps you writing less style related comments :p. Please give me your opinion on this (see #1137). Should we have Evans coding style along with the other pre-sets? (Jquery, Crockford, Airbnb, Google, Grunt, Idiomatic, etc..) or should we conform to one of the presets. If you want respond, issue #1137. Nice vid from Douglas Crockford about this https://www.youtube.com/watch?v=_EANG8ZZbRs .

I'll fix the style comments asap, and replace this pull request with a new 'cleaned up' one (with squashed commits, I just figured out a way to do that).

@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Dec 22, 2015

Member

@profOnno I wouldn't spend a lot of effort on connect-databank. If we eventually move to Express 4.x, we'll probably be ditching Connect anyway. So if I were you, I'd send the bare minimum needed to get tests passing.

Also, don't send a new PR. If you push to the same branch, GitHub will automatically update the commits in this PR. (You may have to force-push with -f, but that's okay.)

Member

strugee commented Dec 22, 2015

@profOnno I wouldn't spend a lot of effort on connect-databank. If we eventually move to Express 4.x, we'll probably be ditching Connect anyway. So if I were you, I'd send the bare minimum needed to get tests passing.

Also, don't send a new PR. If you push to the same branch, GitHub will automatically update the commits in this PR. (You may have to force-push with -f, but that's okay.)

Menno Vossen added some commits Jan 8, 2016

Menno Vossen
Menno Vossen
},
function(err, br) {
var self = this;

This comment has been minimized.

@strugee

strugee Jan 9, 2016

Member

Again, is this actually used?

@strugee

strugee Jan 9, 2016

Member

Again, is this actually used?

This comment has been minimized.

@profOnno

profOnno Jan 10, 2016

In the last function as callback
br.visit("bla",this)... calls next function in Step. self(!br.success, br); calls outer Step

@profOnno

profOnno Jan 10, 2016

In the last function as callback
br.visit("bla",this)... calls next function in Step. self(!br.success, br); calls outer Step

This comment has been minimized.

@ghost

ghost Jan 10, 2016

Please can you un-subscribe my email address shiju.varghese@dekhdekh.com mailto:shiju.varghese@dekhdekh.com so that I do not receive emails like the one below?

Many thanks.

On 10 Jan 2016, at 10:42, Menno Vossen notifications@github.com wrote:

In test/oauth-authorization-test.js #1136 (comment):

                 },
                 function(err, br) {
  •                    var self = this;
    
    In the last function as callback
    br.visit("bla",this)... calls next function in Step. self(!br.success, br); calls outer Step


Reply to this email directly or view it on GitHub https://github.com/e14n/pump.io/pull/1136/files#r49274288.

@ghost

ghost Jan 10, 2016

Please can you un-subscribe my email address shiju.varghese@dekhdekh.com mailto:shiju.varghese@dekhdekh.com so that I do not receive emails like the one below?

Many thanks.

On 10 Jan 2016, at 10:42, Menno Vossen notifications@github.com wrote:

In test/oauth-authorization-test.js #1136 (comment):

                 },
                 function(err, br) {
  •                    var self = this;
    
    In the last function as callback
    br.visit("bla",this)... calls next function in Step. self(!br.success, br); calls outer Step


Reply to this email directly or view it on GitHub https://github.com/e14n/pump.io/pull/1136/files#r49274288.

This comment has been minimized.

@profOnno

profOnno Jan 10, 2016

@ShijuDekhDekh1, I think you have to do that yourself. If you are logged in in github, clicked the link in the email (view it on github) to the origin of the notification. Then you should be on the github page where the notification came from. On the right there should be a button Unsubscribe.

@profOnno

profOnno Jan 10, 2016

@ShijuDekhDekh1, I think you have to do that yourself. If you are logged in in github, clicked the link in the email (view it on github) to the origin of the notification. Then you should be on the github page where the notification came from. On the right there should be a button Unsubscribe.

@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Jan 12, 2016

Member

@profOnno whoops, went through the updated code but forgot to comment! All the changes you've made look awesome; I only see a couple things left - like this and the .fill() stuff (there isn't really a precedent in the codebase, AFAICT, but virtually every other JS project I've seen uses the style I've suggested).

Based on the Travis log it looks to me like the removal of SIGKILL is exposing another failure in the test suite, so we should maybe take a look at that

Lastly, we still shouldn't ship patches in the repo. Here's what I propose: if you remove the connect-databank patch and related scripts from this PR, then I'll go ahead and merge it. I know that means the test suite will still fail, but that way we don't have to worry about the rest of this PR sitting untouched while upstream connect-databank gets fixed. Does that sound OK?

Member

strugee commented Jan 12, 2016

@profOnno whoops, went through the updated code but forgot to comment! All the changes you've made look awesome; I only see a couple things left - like this and the .fill() stuff (there isn't really a precedent in the codebase, AFAICT, but virtually every other JS project I've seen uses the style I've suggested).

Based on the Travis log it looks to me like the removal of SIGKILL is exposing another failure in the test suite, so we should maybe take a look at that

Lastly, we still shouldn't ship patches in the repo. Here's what I propose: if you remove the connect-databank patch and related scripts from this PR, then I'll go ahead and merge it. I know that means the test suite will still fail, but that way we don't have to worry about the rest of this PR sitting untouched while upstream connect-databank gets fixed. Does that sound OK?

@profOnno

This comment has been minimized.

Show comment
Hide comment
@profOnno

profOnno Jan 12, 2016

Ok, for the indent part I'll go with your opinion (will do the alignment manual now.. I think it can be done in vim, but couldn't find how at the moment 🐒 ).

this. You are right.. The closing ')' of Step.

I'll add SIGKILL and do a rebase only the last two commits (clean-up, so there is no remove SIGKILL and add SIGKILL, and multiple clean-up commits).

The connect-databank patch can be removed because I'll use the connect-databank from my github (I need the changes... test will fail without). The other patch needs to remain (check the script to see what it does). Newer versions of connect (not yet compatible) have capped the qs version, like i did in the patch.

To be clear. There were 2 patches, one for connect-databank to stop the timers, and one for connect to use an older version of qs.

I'll do the rebase (commit cleanup merges) later.

profOnno commented Jan 12, 2016

Ok, for the indent part I'll go with your opinion (will do the alignment manual now.. I think it can be done in vim, but couldn't find how at the moment 🐒 ).

this. You are right.. The closing ')' of Step.

I'll add SIGKILL and do a rebase only the last two commits (clean-up, so there is no remove SIGKILL and add SIGKILL, and multiple clean-up commits).

The connect-databank patch can be removed because I'll use the connect-databank from my github (I need the changes... test will fail without). The other patch needs to remain (check the script to see what it does). Newer versions of connect (not yet compatible) have capped the qs version, like i did in the patch.

To be clear. There were 2 patches, one for connect-databank to stop the timers, and one for connect to use an older version of qs.

I'll do the rebase (commit cleanup merges) later.

@profOnno

This comment has been minimized.

Show comment
Hide comment
@profOnno

profOnno Jan 12, 2016

strugee... If you comment on the PR and not on the commits I can merge of the last commits to some grouped ones... so change the history to a cleaner history. Starting from this commit.

profOnno commented Jan 12, 2016

strugee... If you comment on the PR and not on the commits I can merge of the last commits to some grouped ones... so change the history to a cleaner history. Starting from this commit.

@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Jan 13, 2016

Member

@profOnno GitHub should preserve comments on the diff even if you push a squashed commit. Go right ahead.

Member

strugee commented Jan 13, 2016

@profOnno GitHub should preserve comments on the diff even if you push a squashed commit. Go right ahead.

@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Jan 13, 2016

Member

@profOnno with the exception of the patches, which seem really hacky, this looks good to go for me. Nice work!

I asked @cwebber on IRC if we can merge the patch stuff in anyway, with the assumption that it'll go away soonish. We'll see what he says.

Member

strugee commented Jan 13, 2016

@profOnno with the exception of the patches, which seem really hacky, this looks good to go for me. Nice work!

I asked @cwebber on IRC if we can merge the patch stuff in anyway, with the assumption that it'll go away soonish. We'll see what he says.

@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Jan 13, 2016

Member

@profOnno so, @cwebber says that he's fine with the connect-databank patch going in (i.e. the package.json entry pointing to your GitHub), but not the Connect patch (i.e. the stuff in patch/).

Can you remove the Connect patch from this PR, so I can merge it? I understand that the tests will continue to fail with this change, but we can upgrade Connect in a followup PR, and in the meantime the vast majority of the testsuite failures will be fixed.

Member

strugee commented Jan 13, 2016

@profOnno so, @cwebber says that he's fine with the connect-databank patch going in (i.e. the package.json entry pointing to your GitHub), but not the Connect patch (i.e. the stuff in patch/).

Can you remove the Connect patch from this PR, so I can merge it? I understand that the tests will continue to fail with this change, but we can upgrade Connect in a followup PR, and in the meantime the vast majority of the testsuite failures will be fixed.

@profOnno

This comment has been minimized.

Show comment
Hide comment
@profOnno

profOnno Jan 13, 2016

Should be ok now, with no patch folder and a patched connect fork on github. Thought the git rebase -i, to clean only works local?

profOnno commented Jan 13, 2016

Should be ok now, with no patch folder and a patched connect fork on github. Thought the git rebase -i, to clean only works local?

@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Jan 14, 2016

Member

@profOnno awesome!

After you do git rebase -i you have to force-push to GitHub again.

Member

strugee commented Jan 14, 2016

@profOnno awesome!

After you do git rebase -i you have to force-push to GitHub again.

strugee added a commit that referenced this pull request Jan 18, 2016

Merge pull request #1136 from profOnno/r4p
Make the tests work again

@strugee strugee merged commit 729aa1d into pump-io:master Jan 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@strugee

This comment has been minimized.

Show comment
Hide comment
@strugee

strugee Jan 18, 2016

Member

@profOnno thanks so much for all the work you've done here! It's fantastic.

Member

strugee commented Jan 18, 2016

@profOnno thanks so much for all the work you've done here! It's fantastic.

@profOnno

This comment has been minimized.

Show comment
Hide comment
@profOnno

profOnno Jan 18, 2016

I like it. Now only get my pump back up. I think I have a leveldb problem or something, hope I have some time to get it up again, maybe switch to my new CoreOS server and use my pump.ninja domain.

The red crosses next to the commits are from the CLAhub.. How can we get rid of them? (The CLA was removed in favour of DCO)

profOnno commented Jan 18, 2016

I like it. Now only get my pump back up. I think I have a leveldb problem or something, hope I have some time to get it up again, maybe switch to my new CoreOS server and use my pump.ninja domain.

The red crosses next to the commits are from the CLAhub.. How can we get rid of them? (The CLA was removed in favour of DCO)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment