[WIP] Png output no rxjs #24098
[WIP] Png output no rxjs #24098
Conversation
Change PNG button name from Download to Generate
Change PNG button name from Download to Generate
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
timroes
added
Feature:Reporting
|
Jenkins, test this |
| } | ||
|
|
||
| page.on('console', line => { | ||
| if (line._type === 'error') { |
There was a problem hiding this comment.
Could we potentially extract this function into it's own utility and if possible flatten it, since 3 levels of nested if is quiet some nesting :-)
Also we are usually putting the closing } to the next line.
| if (line._text.includes('error while loading shared libraries: libnss3.so')) { | ||
| throw new Error(`You must install nss for Reporting to work`);} | ||
| if (line._text.includes('Check failed: InitDefaultFont(). Could not find the default font')) { | ||
| throw new Error('You must install freetype and ttf-font for Reporting to work');} |
There was a problem hiding this comment.
These error messages are currently really focused around *NIX systems. Not sure if you wouldn't get the same error message on a windows system? I think we should try to produce the same errors on Windows and check if chromium will report the same messages. If they are different I think we can perfectly fine use those *NIX specific messages, if not we should come up with better messages. Maybe we should also link to the reporting setup documentation here.
| return elementsPositionAndAttributes; | ||
| }; | ||
|
|
||
| const getScreenshots = async ({ browser, elementsPositionAndAttributes }) => { | ||
| const getScreenshots = async (browser, elementsPositionAndAttributes) => { |
There was a problem hiding this comment.
I don't see a reason to remove the "named parameters" here? Usually it's easier to read the method calls that way, since you are not relying on any "arbitrary" ordering.
|
@stacey-gammon This touches a lot of reporting code, so someone with a deeper reporting understanding would require to review this. Also it seems to include the reporting to PNG code. @bgaddis56 Is this completely based on top of the PNG exporting PR, so that this can safely be blocked until the PNG reporting is merged? |
|
Yes this PR is completely based on top of the PNG export PR and can wait till that PR is merged. |
💔 Build Failed |
|
There is currently an issue with killing off the child chromium process after the report is finished as well as when there is a timeout or report error. I am in the process of trying to fix this so until then I would wait on reviewing this any further. Thx |
|
It's been a while since we've seen any activity on this PR (and there's now numerous conflicts in our files). Is this something still actively being pursued? If not, I'll pursue closing this in a few days, but if so please comment back @bgaddis56! |
💚 Build Succeeded |
|
Closing due to inactivity |
Summary
Refactored reporting so the code was easier to follow which included removal of RxJs