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

Do not handle exceptions thrown before initialization (closes #493) #494

Closed
wants to merge 2 commits into from

Conversation

strk
Copy link

@strk strk commented Jul 4, 2012

No description provided.

@tj
Copy link
Contributor

tj commented Jul 4, 2012

hmm I dont think we should every have this case, also you have a typo :p

@tj
Copy link
Contributor

tj commented Jul 4, 2012

I can't reproduce this myself but I think this would just mask a real bug so I'm hesitant to merge this, we should find the real issue

@strk
Copy link
Author

strk commented Jul 4, 2012

On Wed, Jul 04, 2012 at 08:45:47AM -0700, TJ Holowaychuk wrote:

I can't reproduce this myself but I think this would just mask a real bug so I'm hesitant to merge this, we should find the real issue

Again I think this was with node-0.8.0

--strk;

@tj
Copy link
Contributor

tj commented Jul 4, 2012

I'm using 0.8.0 but even if that's the case it's a side-effect of another bug

@strk
Copy link
Author

strk commented Jul 4, 2012

On Wed, Jul 04, 2012 at 09:08:12AM -0700, TJ Holowaychuk wrote:

I'm using 0.8.0 but even if that's the case it's a side-effect of another bug

Yeah, could also be a big mess I'm making by switching continuosly
between node versions and dependent modules versions.

Anyway the aim of the commit was to help at debugging the specific
issue/bug, and it was surely more effective than reporting the
"local" error (rather than the error hold by the thrown exception).

--strk;

@ghettodev
Copy link

The error didn't show up for me until I upgraded from 0.8.1 to 0.8.2 from chris-lea-node_js repo on 12.04.

Here is a list of packages I have installed:

├─┬ assert@0.4.9
│ └─┬ util@0.4.9
│   └── events.node@0.4.9
├── chai@1.1.1
├── coffee-script@1.3.3 extraneous
├── connect-flash@0.1.0
├─┬ express@2.5.11
│ ├─┬ connect@1.9.2
│ │ ├── formidable@1.0.11
│ │ ├── mime@1.2.6
│ │ └── qs@0.5.0
│ ├── mime@1.2.4
│ ├── mkdirp@0.3.0
│ └── qs@0.4.2
├─┬ jade@0.26.3
│ ├── commander@0.6.1
│ └── mkdirp@0.3.0
├── less@1.3.0
├─┬ less-middleware@0.1.4
│ └── mkdirp@0.3.3
├── libxmljs@0.5.4
├─┬ mocha@1.3.0
│ ├── commander@0.6.1
│ ├── debug@0.7.0
│ ├── diff@1.0.2
│ └── growl@1.5.1
├─┬ mongoose@2.7.1
│ ├── hooks@0.2.1
│ └─┬ mongodb@1.0.2
│   └── bson@0.0.6
├── node-dev@0.2.4
├─┬ passport@0.1.12
│ └── pkginfo@0.2.3
├─┬ passport-google@0.2.0
│ ├─┬ passport-openid@0.2.3
│ │ └── openid@0.4.2
│ └── pkginfo@0.2.3
├─┬ passport-local@0.1.3
│ └── pkginfo@0.2.3
├─┬ passport-twitter@0.1.3
│ ├─┬ passport-oauth@0.1.9
│ │ └── oauth@0.9.7
│ └── pkginfo@0.2.3
├── request@2.9.203
└── should@0.6.3

Let me know if there is any other debug information you might need.

@strk
Copy link
Author

strk commented Jul 18, 2012

@ghettodev do the commits in this branch fix the issue for you ? By "fix the issue" I mean you get a meaningful error message...

@tj
Copy link
Contributor

tj commented Jul 18, 2012

I've only seen this once and it was because I didn't have redis fired up and tried to run tests, so that makes me think we are only really getting this under similar conditions when mocha is not even really running yet, so we should re-throw to expose the original error and bail out of mocha

@ghettodev
Copy link

I think I've narrowed it down. When I remove the line if (typeof(runnable) == 'undefined') return; provided by strk and run a single instance of node with the command /node_modules/.bin/mocha --compilers coffee:coffee-script I don't receive an error. But if I run npm start in another window and then run the mocha command I get the error: if ('failed' == runnable.state) return;.

When I add strk's addition I can run ./node_modules/.bin/mocha --compilers coffee:coffee-script -G -w -u bdd and node-dev server.js and see the expected output in each tmux window.

@tj
Copy link
Contributor

tj commented Jul 18, 2012

we shouldn't just be ignoring errors though, that's a sign of a larger problem

@strk
Copy link
Author

strk commented Jul 18, 2012

On Wed, Jul 18, 2012 at 10:00:58AM -0700, TJ Holowaychuk wrote:

we shouldn't just be ignoring errors though, that's a sign of a larger problem

I think he mentioned he's seeing the error (it's not ignored) ?
I think I saw it, with my patch, rather than seeing an unrelated error.

--strk;

@ghettodev
Copy link

Although now I wonder if I'm doing this correctly... I'm new to node and am still running through tutorials. Should mocha test and my node dev environment be listening on a different ports?

@ghettodev
Copy link

Sorry if I'm not being clear about the error. When I run node in one window and then run mocha in another window, mocha returns this:

ghettodev@thing ~/w/web  (master) > ./node_modules/.bin/mocha --compilers coffee:coffee-script -G 


/home/ghettodev/workspace/web/node_modules/mocha/lib/runner.js:417
  if ('failed' == runnable.state) return;
                          ^
TypeError: Cannot read property 'state' of undefined
    at Runner.uncaught (/home/ghettodev/workspace/web/node_modules/mocha/lib/runner.js:417:27)
    at process.uncaught (/home/ghettodev/workspace/web/node_modules/mocha/lib/runner.js:450:10)
    at process.EventEmitter.emit (events.js:115:20)

When I shut down the node app and only run mocha I don't see the error. When I add srk's fix and run both instances I don't see the error and both instances work as expected.

@tj tj closed this in b255074 Jul 18, 2012
@strk strk deleted the uncaught branch March 7, 2013 12:31
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.

None yet

4 participants