fix: handle wasm module loading failure and improve logging and perfo… #13
Conversation
|
I've been using |
There was a problem hiding this comment.
Hey, thanks for the PR, and sorry for the delay.
Let me add some context to the fmt.Printf decision. The idea is that this tool should print the log lines exactly the way "go test" runs a test. i.e. you should not realize a browser is running the test for you. If we change to logger.Printf, that defeats the purpose.
Secondly, I totally agree that it's a bug when the test doesn't stop if the instantiation fails. But I'm not sure if your solution is the right approach here. A console error is an error, which should be logged, not an exception. What I had in mind was to actually throw an exception if the file didn't load/initialize properly, then do a chromedp.Cancel(ctx) in the *cdpruntime.EventExceptionThrown case. I don't see any reason to keep the program running if a javascript exception was thrown.
Regarding adding debug logging, it's something that's definitely needed. But I don't want to add yet another env variable for it. Instead, let's overload the -v flag for it. And have a deeper option like -v=2 to enable the debug logging.
Thoughts ?
|
I don't have many strong opinions about how error logging is done so (1) is
fine as long as what you've suggested will eventually be compatible with
making the output JSON just like how I think there's a JSON flag for go
test. It's a bit easier to redirect logger calls imo but I'd rather see the
code accepted / written quickly than worry about it.
I used a console error because it's a recoverable and even expected case I
want to be able to test for my E2E tests - I've added some JS to show a
banner if the loading fails in a different project and I'll likely be
trying to save partially downloaded wasm code and stuff in browser storage
some day. But for wasmbrowsertest an exception could be more appropriate.
I'm mostly unfamiliar with JS best coding practices and saw them as
equivalent, but since I think of the error as recoverable I wrote it with
console. I can change to an exeption.
That's fine unless the flag is a boolean and you're suggesting changing it
to an int.
Overall I think it'll be faster for you to just reject this PR and author
your own focused on adding debugging. You might also take out network layer
debugging, although as I said elsewhere I found it useful.
…On Sat, Feb 22, 2020, 2:44 PM Agniva De Sarker ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Hey, thanks for the PR, and sorry for the delay.
Let me add some context to the fmt.Printf decision. The idea is that this
tool should print the log lines exactly the way "go test" runs a test. i.e.
you should not realize a browser is running the test for you. If we change
to logger.Printf, that defeats the purpose.
Secondly, I totally agree that it's a bug when the test doesn't stop if
the instantiation fails. But I'm not sure if your solution is the right
approach here. A console error is an error, which should be logged, not an
exception. What I had in mind was to actually throw an exception if the
file didn't load/initialize properly, then do a chromedp.Cancel(ctx) in the
*cdpruntime.EventExceptionThrown case. I don't see any reason to keep the
program running if a javascript exception was thrown.
Regarding adding debug logging, it's something that's definitely needed.
But I don't want to add yet another env variable for it. Instead, let's
overload the -v flag for it. And have a deeper option like -v=2 to enable
the debug logging.
Thoughts ?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#13?email_source=notifications&email_token=ADB57W7IXOAW3SX53CKDCJDREGFD5A5CNFSM4KQNHDJKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWR3IFY#pullrequestreview-363050007>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADB57W4GBS5E3IL5AKHDEB3REGFD5ANCNFSM4KQNHDJA>
.
|
Not sure I fully understand this. I was talking about fmt.Printf vs log.Printf. The debug logging is in JSON but that's a different topic.
Yes that's the point. But here you were using a console error and making it irrecoverable. That's why I wanted to change it to an exception. |
|
For JSON, I was thinking about It would be nice if the output of wasmbrowsertest could also be piped to test2json including debugging information. I have no personal need for that, but I imagine someone will want it some day. I switched over to logger in part because I imagine some day the print calls will get wrapped in a function call which either logs as it currently does or logs in a way which is compatible with test2json, and at that point I assume someone would refactor the code to all use logger calls or all use fmt calls - keeping both is possible but seems odd to me to log some errors in handleEvent with fmt.Printf but others - like chromedp.Cancel errors - with logger.Printf. I didn't realize console.xyz is all the same thing just with different styling - I just assumed console.error did something more useful than changing the printed color in the console. Anyway throwing an exception and handling it in handleEvent sounds fine. Verbose-flag wise, it looks like a boolean in I'd rather give developers basically no output (as currently happens) and then when you add -v just dump out everything a developer could possibly need. |
|
Yeah let me put some more thought into the Closing this one for now then. Thanks very much for the discussion and the ideas. |
Problems
Sorted from real problem to nice-to-have:
chromedp.WaitEnabledcommand until the test times out.Solutions
case *cdpruntime.EventConsoleAPICalledit was assumed the error would be a number. I can add a new case specifically for console.Error which helps with this.I also created a 'DEBUG' option which contained the information I needed to debug the problems I was running in to; presumably such a thing will be helpful for someone else in the future.
What the changes look like on WSL, where the program seems to hang
With DEBUG=on
With DEBUG=off it can seem to hang:
Running on Windows, where the program works
DEBUG=off
DEBUG=on
output continued...