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.

Sign up for GitHub

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

Jump to bottom

More compatible with iojs #560

Merged
merged 5 commits into from
Apr 21, 2015
Merged

More compatible with iojs #560

merged 5 commits into from
Apr 21, 2015

Conversation

3y3
Copy link
Member

@3y3 3y3 commented Feb 15, 2015

We are not full compatible, but can use old features.

@bajtos , please review.

@3y3
Copy link
Member Author

3y3 commented Feb 15, 2015

Sorry, not ready to review

@bajtos
Copy link
Member

bajtos commented Feb 17, 2015

Sorry, not ready to review

No worries, please leave a comment when it's ready.

@3y3 3y3 force-pushed the more-iojs branch 2 times, most recently from f26560e to 2383912 Compare February 19, 2015 20:13
@3y3 3y3 force-pushed the more-iojs branch 2 times, most recently from dff252c to ff49529 Compare March 5, 2015 21:22
@3y3 3y3 force-pushed the more-iojs branch 2 times, most recently from bb4f93b to f7355a7 Compare March 21, 2015 20:03
@3y3 3y3 mentioned this pull request Mar 21, 2015
Closed
@3y3 3y3 force-pushed the more-iojs branch 4 times, most recently from 68904da to 1596417 Compare March 30, 2015 21:33
@3y3
Copy link
Member Author

3y3 commented Apr 14, 2015

@bajtos , this partially ready to review.

Strategy of this pr:

  1. Refactor session module. Inherit it from EventEmitter.
  2. Restructure all modules to receive session object instead of debuggerClient, frontendClient etc.
  3. Fix test after refactoring
  4. Fix test for 0.12 - to be sure that we work on 0.12 with new changes (there are some speculative changes. I need help to understand how they work)
  5. Emit resource-tree event from session instead of PageAgent. Rename to resource-tree-resolved
  6. Add new tests for resource-tree-resolved feature

About fixes for 0.12:
Ok, I can understand why we need to resume debugger after injections like there - this is related to new isolated debugger with his own event loop.
But that happens here, how it's possible what fn is undefined?
I inspect this situation long time:

  • If you try to pass all tests without this change - failure any time
  • If you try to pass only tests for InjectorClient without this change - failure some time
  • If you try to pass only failing test without this change - all works. WTF?!

It may be that we need help of core node developers here. Unfortunately I can't reproduce this problem in isolated sample.

bajtos
bajtos reviewed Apr 14, 2015
View reviewed changes
test/BreakEventHandler.js
session;

describe('should cache events', function() {
it('if started in debug-brk mode', function(done) {
Copy link
Member

Choose a reason for hiding this comment

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

The test names "describe should change events" and "it if started in debug-brk mode" don't read as English. Can you please rephrase? For example:

describe('event cache', function() { // or 'event caching'
  it('works correctly when started in debug-brk mode', function(done) {

The same applies to all other test names.

@bajtos
Copy link
Member

bajtos commented Apr 14, 2015

I like the idea of moving the different object instances to a single session object. My initial dependency-injection approach did not worked well. Can you please make SessionStub more feature complete, so that it can be used as a drop-in replacement for a real Session object in as many tests as possible?

The changes made in the production code (lib/*) look good to me.

About fixes for 0.12:
Ok, I can understand why we need to resume debugger after injections like there - this is related to new isolated debugger with his own event loop.
But that happens here, how it's possible what fn is undefined?

Perhaps debug-brk stops the process before console.log is defined?

@bajtos bajtos assigned 3y3 and unassigned bajtos Apr 14, 2015
@3y3
Copy link
Member Author

3y3 commented Apr 14, 2015

Can you please make SessionStub more feature complete, so that it can be used as a drop-in replacement for a real Session object in as many tests as possible

Can you, please, describe more verbose this part?

@bajtos
Copy link
Member

bajtos commented Apr 14, 2015

Can you please make SessionStub more feature complete, so that it can be used as a drop-in replacement for a real Session object in as many tests as possible

Can you, please, describe more verbose this part?

Sure, see #560 (comment)

@3y3 3y3 mentioned this pull request Apr 17, 2015
Closed
@3y3 3y3 force-pushed the more-iojs branch 2 times, most recently from c13672c to 87a751f Compare April 21, 2015 07:49
@3y3
Copy link
Member Author

3y3 commented Apr 21, 2015

Added description for tests. Rebased to master.
Ready to review.

@3y3 3y3 assigned bajtos and unassigned 3y3 Apr 21, 2015
@3y3
Copy link
Member Author

3y3 commented Apr 21, 2015

I'll change names on next fix step.
Waiting resolution about FrontendCommandHandler test.

@bajtos
Copy link
Member

bajtos commented Apr 21, 2015

No more comments. I'd really like to see waitResourceTree renamed because it does not follow English grammar IMHO, but I can live with the current version too.

@bajtos bajtos assigned 3y3 and unassigned bajtos Apr 21, 2015
@3y3
Copy link
Member Author

3y3 commented Apr 21, 2015

@bajtos , I'm really sorry for my English =) You feel free to correct me any time.

@3y3
Copy link
Member Author

3y3 commented Apr 21, 2015

Ok. I'll make last fixes and merge this.

3y3 added 2 commits April 21, 2015 15:31
@3y3
Cache events from app
3727b32 
Cache events from app before resource tree resolving
@3y3
Add tests for BreakEventHandler
9276e21
@3y3 3y3 merged commit 9276e21 into node-inspector:master Apr 21, 2015
@3y3 3y3 deleted the more-iojs branch April 22, 2015 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@3y3 3y3

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@3y3 @bajtos