Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

fix(gulp): unhandled error (not printed for the user) #1261

Merged
merged 1 commit into from
Mar 18, 2016
Merged

fix(gulp): unhandled error (not printed for the user) #1261

merged 1 commit into from
Mar 18, 2016

Conversation

lirantal
Copy link
Member

gulpfile.js had an handled error, or more precisely handled but the actual error wasn't printed to the screen.

@lirantal lirantal self-assigned this Mar 10, 2016
@lirantal lirantal added this to the 0.5.0 milestone Mar 10, 2016
@mleanos
Copy link
Member

mleanos commented Mar 10, 2016

LGTM.

ilanbiala
ilanbiala reviewed Mar 11, 2016
View reviewed changes
gulpfile.js
@@ -327,6 +327,7 @@ gulp.task('protractor', ['webdriver_update'], function () {
process.exit(0);
})
.on('error', function(err) {
console.log(err);
Copy link
Member

Choose a reason for hiding this comment

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

.error maybe? Also, why don't we put the E2E failing above it to give some context.

Copy link
Member Author

Choose a reason for hiding this comment

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

we use consistently console.log() on gulp/grunt. Should I change all of them to .error for consistency?

Copy link
Member

Choose a reason for hiding this comment

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

If it's an error, then yeah I think it makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

cool, I'll update.

@lirantal lirantal force-pushed the bugfix/gulpfile_lint_issues branch from e40fcf0 to 5a6d954 Compare March 12, 2016 14:25
@lirantal
Copy link
Member Author

@ilanbiala updated .error() related statements.

ilanbiala
ilanbiala reviewed Mar 12, 2016
View reviewed changes
gulpfile.js
@@ -327,7 +327,8 @@ gulp.task('protractor', ['webdriver_update'], function () {
process.exit(0);
})
.on('error', function(err) {
console.log('E2E Tests failed');
console.error(err);
console.error('E2E Tests failed');
Copy link
Member

Choose a reason for hiding this comment

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

This should be above the error to give context.

Copy link
Member

Choose a reason for hiding this comment

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

@lirantal can you reorder these two statements before you merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, in this case I'll add a colon to the E2E Tests failed text too to make it clear

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sounds good.

@ilanbiala
Copy link
Member

Reran the build, hopefully it passes.

@lirantal
Copy link
Member Author

Obviously the build doesn't fail due to the console.error() so apparently we have a bigger problem there? Looks related to protractor.

@ilanbiala
Copy link
Member

Yeah, weird that Node 5 passes though...@mleanos @trendzetter @codydaig any ideas?

@mleanos
Copy link
Member

mleanos commented Mar 12, 2016

I first saw this issue last night when I submitted a PR. I've been looking into it for a couple hours.

I went through one of the failed builds log, and compared it to a recent successful build we had. I was looking for differences in the packages installed, and any variations in the Travis environment. I couldn't find any differences that are relevant.

Now I'm getting the same exact error with both grunt test & gulp test locally, after blowing away node_modules, npm cache clean & npm install.

I'll start looking at the protractor project issues.

@mleanos
Copy link
Member

mleanos commented Mar 12, 2016

Are you getting a TypeError: Cannot read property 'dropDatabase of undefined error thrown when running either grunt test:e2e or gulp test:e2e? It started showing up for me after I did a clean install of the project's node packages. Could be related.

@mleanos
Copy link
Member

mleanos commented Mar 12, 2016

Seems to be an issue with Mongoose 4.4.7 which was released 23 hours ago. I installed 4.4.6 and the E2E tests are fine. It's kind of interesting that we didn't see anything with this in the logs that indicated the exception thrown on the dropdb task.

@mleanos
Copy link
Member

mleanos commented Mar 12, 2016

Let's see if the builds succeed in #1264

@mleanos
Copy link
Member

mleanos commented Mar 13, 2016

Re-ran the build since I merged in #1264

@lirantal
Copy link
Member Author

Ok so I'm merging this one but for #1264 we need to investigate it further.

@ilanbiala
Copy link
Member

@lirantal LGTM except one change that I would suggest that we reorder.

@lirantal lirantal force-pushed the bugfix/gulpfile_lint_issues branch from 5a6d954 to 6e9dc4c Compare March 14, 2016 06:39
@lirantal
Copy link
Member Author

updated re-ordering.

@mleanos
Copy link
Member

mleanos commented Mar 14, 2016

LGTM. So the idea is to start using console.error in cases like this, throughout the framework?

@lirantal
Copy link
Member Author

No, not through-out the framework, just in gulpfile.js.
For the framework we have a logging system (which I need to update)

On Mon, Mar 14, 2016 at 8:57 AM, Michael Leanos notifications@github.com
wrote:

LGTM. So the idea is to start using console.error in cases like this,
throughout the framework?

—
Reply to this email directly or view it on GitHub
#1261 (comment).

Sincerely, Liran Tal

Author
http://www.amazon.com/Agile-Software-Development-HP-Manager/dp/1484210352/of
Agile Software Development with HP Agile Manager
Founder and Lead Developer of daloRADIUS http://www.daloradius.com/
Blogging at http://www.enginx.com, and tweeting at @liran_tal
https://twitter.com/liran_tal

@ilanbiala
Copy link
Member

LGTM.

@rhutchison
Copy link
Contributor

LGTM

@ilanbiala
Copy link
Member

@lirantal please rebase and the build should still pass barring any ESLint issues.

@lirantal
Copy link
Member Author

I WILL NOT REBASE!!!

@mleanos
Copy link
Member

mleanos commented Mar 17, 2016

RE-BRACE yo-self foo!

@lirantal
Copy link
Member Author

whaaaaat? who you calling a foo?!

@lirantal lirantal force-pushed the bugfix/gulpfile_lint_issues branch from 6e9dc4c to b4c9464 Compare March 17, 2016 20:32
@codydaig
Copy link
Member

LGTM

@ilanbiala
Copy link
Member

LGTM.

lirantal added a commit that referenced this pull request Mar 18, 2016
@lirantal
Merge pull request #1261 from lirantal/bugfix/gulpfile_lint_issues
8d137ac 
fix(gulp): unhandled error (not printed for the user)
@lirantal lirantal merged commit 8d137ac into meanjs:master Mar 18, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@lirantal lirantal

Labels
issue:bug
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.
5 participants
@lirantal @mleanos @ilanbiala @rhutchison @codydaig