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

promise tests failing under recent versions of iojs #33

Open
othiym23 opened this issue Mar 7, 2015 · 3 comments
Open

promise tests failing under recent versions of iojs #33

othiym23 opened this issue Mar 7, 2015 · 3 comments

Comments

@othiym23
Copy link
Owner

othiym23 commented Mar 7, 2015

Hey @Qard, I'd like to make this your problem if I could, since you wrote the original code. Under at least [email protected] and [email protected], the promise tests show a bunch of failures where they're getting deeper trace trees than they were expecting. Also, something's causing things to go bozotic with EEs in the test:

(node) warning: possible EventEmitter memory leak detected. 11 close listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at Stream.addListener (events.js:236:17)
    at Stream.pipe (stream.js:52:12)
    at module.exports (/Users/ogd/Documents/projects/async-listener/node_modules/tap/node_modules/difflet/node_modules/charm/index.js:42:15)
    at difflet (/Users/ogd/Documents/projects/async-listener/node_modules/tap/node_modules/difflet/index.js:41:17)
    at Function.fn.compare (/Users/ogd/Documents/projects/async-listener/node_modules/tap/node_modules/difflet/index.js:20:9)
    at diffObject (/Users/ogd/Documents/projects/async-listener/node_modules/tap/lib/tap-assert.js:419:50)
    at assert (/Users/ogd/Documents/projects/async-listener/node_modules/tap/lib/tap-assert.js:55:18)
    at Function.equivalent (/Users/ogd/Documents/projects/async-listener/node_modules/tap/lib/tap-assert.js:183:12)
    at Test._testAssert (/Users/ogd/Documents/projects/async-listener/node_modules/tap/lib/tap-test.js:87:16)
    at next (/Users/ogd/Documents/projects/async-listener/test/native-promises.tap.js:497:9)
(node) warning: possible EventEmitter memory leak detected. 11 end listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at Stream.addListener (events.js:236:17)
    at Stream.pipe (stream.js:99:10)
    at module.exports (/Users/ogd/Documents/projects/async-listener/node_modules/tap/node_modules/difflet/node_modules/charm/index.js:42:15)
    at difflet (/Users/ogd/Documents/projects/async-listener/node_modules/tap/node_modules/difflet/index.js:41:17)
    at Function.fn.compare (/Users/ogd/Documents/projects/async-listener/node_modules/tap/node_modules/difflet/index.js:20:9)
    at diffObject (/Users/ogd/Documents/projects/async-listener/node_modules/tap/lib/tap-assert.js:419:50)
    at assert (/Users/ogd/Documents/projects/async-listener/node_modules/tap/lib/tap-assert.js:55:18)
    at Function.equivalent (/Users/ogd/Documents/projects/async-listener/node_modules/tap/lib/tap-assert.js:183:12)
    at Test._testAssert (/Users/ogd/Documents/projects/async-listener/node_modules/tap/lib/tap-test.js:87:16)
    at next (/Users/ogd/Documents/projects/async-listener/test/native-promises.tap.js:497:9)
not ok test/native-promises.tap.js .................... 24/29

Perhaps this is related to the changes that Trevor or vkurchatkin (I think?) landed around the microtask queue in [email protected]? Either way, let me know within a day or two if you're going to tackle these, or if I should budget some time to get my head around this code and fix it myself.

@Qard
Copy link
Collaborator

Qard commented Mar 7, 2015

I'll have a bit of time to look at it tomorrow. I'm not sure what I'll discover. @hayes may also have some ideas, since he expanded a bunch on my initial effort.

@hayes
Copy link
Collaborator

hayes commented Mar 9, 2015

I don't really have time at the moment to dig into this. Looks like some new things are happening under the hood that were not happening in pre 1.3 versions of iojs

@Qard
Copy link
Collaborator

Qard commented Mar 10, 2015

The EventEmitter leak seems to not be a problem with async-listener at all, it seems to originate from tap. Specifically, this line: https://github.com/isaacs/node-tap/blob/master/lib/tap-assert.js#L55

It surfaces when tap tries to use difflet to display a highlighted object diff. I bypassed that by just commenting out that line and doing process._rawDebug(...) calls on json stringified res.found and res.wanted.

The real error is a structural difference in the deep equality check that is produced by the addListner() helper versus what actually gets produced by the manually built object:

// found
found {
  "name": "root",
  "children": [{
    "name": "reject",
    "children": [{
      "name": "nextTick in catch",
      "children": [],
      "before": 1,
      "after": 1,
      "error": 0
    }, {
      "name": "setTimeout in then",
      "children": [],
      "before": 1,
      "after": 0,
      "error": 0
    }],
    "before": 2,
    "after": 2,
    "error": 0
  }, {
    "name": "reject",
    "children": [],
    "before": 1,
    "after": 1,
    "error": 0
  }, {
    "name": "setImmediate in root",
    "children": [],
    "before": 1,
    "after": 1,
    "error": 0
  }],
  "before": 0,
  "after": 0,
  "error": 0
}

// wanted
{
  "name": "root",
  "children": [{
    "name": "reject",
    "children": [{
      "name": "nextTick in catch",
      "children": [],
      "before": 1,
      "after": 1,
      "error": 0
    }, {
      "name": "setTimeout in then",
      "children": [],
      "before": 1,
      "after": 0,
      "error": 0
    }],
    "before": 2,
    "after": 2,
    "error": 0
  }, {
    "name": "setImmediate in root",
    "children": [],
    "before": 1,
    "after": 1,
    "error": 0
  }],
  "before": 0,
  "after": 0,
  "error": 0
}

Note the extra reject in what was found. I'm not yet sure why it does this, just that it is doing it.

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

No branches or pull requests

3 participants