Typescript-ify screenshot stitcher code in reporting #20061
Conversation
stacey-gammon
added
:Sharing
v7.0.0
v6.4.0
(Deprecated) Feature:Reporting
stacey-gammon
force-pushed
the
2018-06-19-typescript-screenshot-stitcher
branch
from
d02a24d
to
0068882
Compare
stacey-gammon
changed the title
| */ | ||
| export function $combine( | ||
| screenshots: Screenshot[], | ||
| outputSize: Size, |
There was a problem hiding this comment.
Changed this to only take in Size, not Dimension, since the x and y were never used.
| * equal to 10, each clip taken, before being zoomed in, should be no bigger than 1 x 1. | ||
| * If zoom is 10 and maxDimensionPerClip is 20, then each clip taken before being zoomed in should be no bigger than 2 x 2. | ||
| * @param captureScreenshotFn - a function to take a screenshot from the page using the dimensions given. The data | ||
| * returned should have dimensions not of the clip passed in, but of the clip passed in multiplied by zoom. |
There was a problem hiding this comment.
This is where the bug is I believe for #19563 - for some reason the captureScreenshotFn is returning data that is too small. We may need to wrap it in a retry, since it seems flaky. Ideally I can figure out why though...
💚 Build Succeeded |
stacey-gammon
force-pushed
the
2018-06-19-typescript-screenshot-stitcher
branch
from
68541e0
to
3b2cdec
Compare
| return parseAsObservable(buffer); | ||
| }, | ||
| (screenshot: Screenshot, png: PNG) => { | ||
| if ( |
There was a problem hiding this comment.
This is new too, this is to detect earlier the bug in #19563. I simulated an error to verify it is exposed correctly:
(Note after that screenshot I made slight adjustments to the message).
|
cc @timroes and @azasypkin - another typescript PR if either of you want to review (not necessary though if you are too busy). |
💔 Build Failed |
|
Hmm Jenkins, test this |
💔 Build Failed |
stacey-gammon
force-pushed
the
2018-06-19-typescript-screenshot-stitcher
branch
2 times, most recently
from
205c40a
to
2a5840e
Compare
💔 Build Failed |
💔 Build Failed |
stacey-gammon
force-pushed
the
2018-06-19-typescript-screenshot-stitcher
branch
from
d6afc41
to
bb5b340
Compare
💚 Build Succeeded |
💔 Build Failed |
|
failure looks unrelated: jenkins, test this |
💚 Build Succeeded |
* typescript screenshot stitcher * Throw an error if the data captured is not of the expected width and height. * Import babel-core types * Add babel-core types to x-pack package.json * Dimensions => Rectangle
|
|
||
| // No types found for this package. May want to investigate an alternative with types. | ||
| // @ts-ignore: implicit any for JS file | ||
| import $streamToObservable from '@samverschueren/stream-to-observable'; |
There was a problem hiding this comment.
note: uggggh, we just should probably write our own analog of https://github.com/Jason3S/rx-stream/blob/master/src/streamToRx.ts at some point.
|
|
||
| // Turn the screenshot data into actual PNGs | ||
| const pngs$ = Rx.from(screenshots).pipe( | ||
| mergeMap( |
There was a problem hiding this comment.
note: it seems mergeMap is deprecated
There was a problem hiding this comment.
Could you please link to that? I don't see any note about deprecation in the docs?
There was a problem hiding this comment.
Sorry, I wasn't precise enough, this form of mergeMap (with resultSelector argument) is deprecated. See:
| const clips = await $getClips(input.rectangle, input.max) | ||
| .pipe(toArray()) | ||
| .toPromise(); | ||
| expect(clips.length).toBe(expectedClips.clips.length); |
There was a problem hiding this comment.
question: can't we just replace this:
expect(clips.length).toBe(expectedClips.clips.length);
for (let i = 0; i < clips.length; ++i) {
expect(clips[i]).toEqual(expectedClips.clips[i]);
}with that?
expect(clips).toEqual(expectedClips.clips);There was a problem hiding this comment.
Those two should be identical.
Should be essentially no logic changes, though I called out one place I did modify an API by not sending in information that wasn't used.
To test, set
xpack.reporting.capture.browser.typetoChromiumand generate some reports.The test suite doesn't yet cover chromium reports so tests won't catch any issues. Need to fix #19563 first, which was what motivated this PR, to try to make more sense of what is going on.