Implement OnLoadingFailed in CDP #156
Conversation
| @@ -17,5 +19,8 @@ | |||
| <body> | |||
| <h1>Common collector test page</h1> | |||
| <p>The content of this HTML is used to validate that all collectors that parse full HTML send the same events.</p> | |||
| <p>These two images are to test that we are not entering in a infinite loop when we have more than one element with the same src or href</p> | |||
| <img src='http://localhost/edge.png'/> | |||
There was a problem hiding this comment.
By default jsdom doesn't download images unless we have the canvas package (at least with v10). We should look into adding it.
https://github.com/tmpvar/jsdom#loading-subresources
There was a problem hiding this comment.
This test is just for CDP. In jsdom we don't have that problem
There was a problem hiding this comment.
Maybe I can add into the message that those images are for testing in CDP collector
There was a problem hiding this comment.
It was more a comment rather than something you should do now.
We will have to do it for some of the tests, probably when we update to v10 :)
src/lib/collectors/cdp/cdp.ts
Outdated
| @@ -94,6 +94,11 @@ class CDPCollector implements ICollector { | |||
| return elements[0]; | |||
| } | |||
|
|
|||
| if (parts.length === 0) { | |||
There was a problem hiding this comment.
Shouldn't this if be taken care of in the last line of the while? If there is something wrong with that then we should:
- Move this if to the top of the
while. - Replace the comment with the one from the bottom:
/* If we reach this point, we have several elements that have the same url so we return the first
because its the one that started the request. */
- Remove the
return elements[0]at the end.
There was a problem hiding this comment.
@molant I'll try to refactor the code to exit the while with it condition, not with that if
src/lib/collectors/cdp/cdp.ts
Outdated
| // TODO: implement this for `fetch::error` and `targetfetch::error`. | ||
| console.error(params); | ||
| private async onLoadingFailed(params) { | ||
| // DOM is not ready so we queued up the event for later |
There was a problem hiding this comment.
// DOM is not ready so we queue up the event for later
src/lib/collectors/cdp/cdp.ts
Outdated
| return; | ||
| } | ||
|
|
||
| /* If the requestId is not in this._requests then that means that we already process the |
There was a problem hiding this comment.
/* If
requestIdis not inthis._requestsit means that we already processed the request inonResponseReceived*/
src/lib/collectors/cdp/cdp.ts
Outdated
| * request in onResponseReceived | ||
| */ | ||
| if (!this._requests.has(params.requestId)) { | ||
| debug(`requestId doesn't exits skipping this error`); |
There was a problem hiding this comment.
`requestId doesn't exist, skipping this error
src/lib/collectors/cdp/cdp.ts
Outdated
| return; | ||
| } | ||
|
|
||
| debug(`Error found: ${JSON.stringify(params)}`); |
There was a problem hiding this comment.
debug(Error found:\n${JSON.stringify(params)});
| @@ -105,6 +105,11 @@ export class Requester { | |||
| return Requester.charsetAliases.get(results[1]) || results[1]; | |||
| } | |||
|
|
|||
| /** Return the redirects for a given `uri` */ | |||
There was a problem hiding this comment.
/** Return the redirects for a given
uri. */
src/lib/types/events.ts
Outdated
| @@ -27,6 +27,8 @@ export interface IFetchErrorEvent { | |||
| element: IAsyncHTMLElement; | |||
| /** The error found. */ | |||
| error: any; | |||
| /** The hop for the url */ | |||
There was a problem hiding this comment.
/** The redirects performed for the url. */
tests/lib/collectors/_common.ts
Outdated
| * | ||
| * We need to add here the `fetch::error` ones | ||
| */ | ||
| /** The minimum set of events the collectors need to implement */ |
There was a problem hiding this comment.
/** The minimum set of events the collectors need to implement. */
tests/lib/collectors/_common.ts
Outdated
| resource: 'http://localhost/script4.js', | ||
| request: { url: 'http://localhost/script4.js' }, | ||
| response: { | ||
| body: { |
There was a problem hiding this comment.
I think you can remove body, and hops. If a property doesn't exist we ignore it when testing.
tests/lib/collectors/_common.ts
Outdated
| @@ -238,6 +278,13 @@ const testCollector = (collectorBuilder: ICollectorBuilder) => { | |||
| invokes.push(emitAsync.getCall(i).args); | |||
| } | |||
|
|
|||
| /* We need to test that there was just a fetch::error */ | |||
There was a problem hiding this comment.
// We test there was just a single
fetch::erroremitted
Fix infinete loop when there is two or more elements with an absolute path in the dom Fix the data we send in JSDOM when an error happens
|
@molant conflicts resolved |
No description provided.