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

Fix: Refactor debugging protocol #1409

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@molant
Member

molant commented Oct 16, 2018

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

Refactor of the debugging protocol code. We were having a few issues and needed some clean up.

Fix #1398

Keep in mind this is a work in progress. I still need to make all the test pass and I want to add some documentation on how the debugging protocol works because I will completely forget in 2 weeks.

@molant molant requested review from antross, alrra and sarvaje Oct 16, 2018

@molant molant force-pushed the molant:fix/scan-issues branch 4 times, most recently from 8dd3bb4 to f46f764 Oct 16, 2018

@molant

This comment has been minimized.

Member

molant commented Oct 17, 2018

@sarvaje @antross I should have addressed all the feedback you gave me in different channels and tests for connector-chrome are now passing in my machine so this should be ready for another review.

@molant

This comment has been minimized.

Member

molant commented Oct 17, 2018

now passing in my machine so this should be ready for another review

And for whatever reason the favicon tests fail in Travis 😭

@molant

This comment has been minimized.

Member

molant commented Oct 17, 2018

And for whatever reason the favicon tests fail in Travis 😭

@alrra could it be that because we are using npm I'm getting the previous version of utils-debugging-protocol and that could be messing things up?

@alrra

This comment has been minimized.

Member

alrra commented Oct 17, 2018

we are using npm

We are using yarn.

npm is only used when releasing packages.

@molant

This comment has been minimized.

Member

molant commented Oct 17, 2018

The problem seems to be that chrome will not download the favicon when running in headless mode.

I think that I'll disable those tests via is-ci if needed. Any objections?

@molant molant force-pushed the molant:fix/scan-issues branch from f46f764 to dc6dc5f Oct 17, 2018

@antross

Nothing blocking, but some thoughts on how to get rid of a couple more ! overrides.

@sarvaje

This comment has been minimized.

Member

sarvaje commented Oct 17, 2018

The problem seems to be that chrome will not download the favicon when running in headless mode.

I think that I'll disable those tests via is-ci if needed. Any objections?

@molant can this be a problem in a scanner using chrome connector?

@molant molant force-pushed the molant:fix/scan-issues branch from a4a6b60 to 429b6ec Oct 17, 2018

@antross

Once again nothing blocking, and my one new comment should likely be left to be tackled in a separate update. Looks good to me otherwise!

@molant

This comment has been minimized.

Member

molant commented Oct 18, 2018

All feedback should be in and hopefully test pass now. At the end I had to force the download of the favicon when running as --headless so there are a few more files changed to expose the flags used to open chrome.
If you can take another look it will be great.

@molant molant force-pushed the molant:fix/scan-issues branch from 366446e to 6d023d0 Oct 18, 2018

@molant

This comment has been minimized.

Member

molant commented Oct 18, 2018

Finally 🎉
image

Show resolved Hide resolved packages/connector-chrome/tests/collect.ts Outdated

molant added some commits Oct 16, 2018

Fix: Refactor debugging protocol
Refactor how the interaction with the debugging protocol is handled
while fixing some of the most recent issues found.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Fix #1398
Fix #1395
Fix #1403
Fix webhintio/rfcs#44

@molant molant force-pushed the molant:fix/scan-issues branch from 6d023d0 to f554878 Oct 18, 2018

@sarvaje

LGTM

@molant

This comment has been minimized.

Member

molant commented Oct 18, 2018

@antross, I've modified a few new files since you last approved it. Can you please take a look?
@alrra do you want to take a look? I'd like to have this merged and published tomorrow Friday and before merging any of the breaking changes so we can do another big run test.
I know you are traveling so I can take care of that if you can't.

@antross

This comment has been minimized.

Member

antross commented Oct 19, 2018

I've modified a few new files since you last approved it. Can you please take a look?

@molant Still looks good to me.

@alrra alrra closed this in 5d8a804 Oct 19, 2018

@molant molant deleted the molant:fix/scan-issues branch Oct 31, 2018

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