Implement RemoteObject conversion to native types #130
Conversation
There was a problem hiding this comment.
Good job! 👏
Overall is good. Maybe we can untangle Logrus from the code to prevent Logrus to propagate into the system much more. I think it's better to separate logging responsibility from parsing.
For example, we don't need to check for: "object will be parsed partially" in parseRemoteObjectPreview using a logger which complicates tests and tangles logging with parsing.
Instead, we can test whether parseRemoteObjectValue returns these error messages, and test against that, and use them in the frame session, here is a rough idea:
func parseRemoteObjectPreview(op *cdpruntime.ObjectPreview) (map[string]interface{}, error) {
var errs error
if op.Overflow {
errs = multierror.Append(errs, errors.New("object will be parsed partially"))
}
obj := make(map[string]interface{})
for _, p := range op.Properties {
val, err := parseRemoteObjectValue(p.Type, p.Value, p.ValuePreview)
if err != nil {
errs = multierror.Append(errs, fmt.Errorf("failed parsing object property '%s'", p.Name))
continue
}
obj[p.Name] = val
}
return obj, errs
}Then in frame_session:
func (fs *FrameSession) onConsoleAPICalled(event *runtime.EventConsoleAPICalled) {
...
for _, robj := range event.Args {
i, err := parseRemoteObject(robj, l)
if err == nil {
parsedObjects = append(parsedObjects, i)
continue
}
if merr, ok := err.(*multierror.Error); ok {
// check if partial parse
// log here
// check if "failed parsing object property"
// log here
continue
}
// If this throws it's a bug :)
k6Throw(fs.ctx, "unable to parse remote object value: %w", err)
}
...Then in tests, you can check against these errors without using a logger.
Btw, can you also convert every k6common.Throw to k6Throw while in here?
So we can make this code into a proper shape :)
ee3a495 to
47a91dc
Compare
47a91dc to
7c2eaae
Compare
|
@inancgumus re: using
If you agree, I can rewrite it with |
Can I ask why? Errors and asserting on errors are the standard way of working in Go. The whole stdlib works like that. Even So, we don't have to use the
I think not everything should be an error to be handled as an error. For example, So far from what I see, I don't like the API of the
👍
I'm trying to convince you that we can use simple errors instead of depending on that package but your call. |
|
OK, I'll change it to a custom error or use |
Previously we used to throw an error if we encountered a custom object
as returned by CDP from an `Evaluate()` or `console.log()` call.
This should handle those cases properly now by converting objects to
`map[string]interface{}` internally, and using a custom Logrus formatter
to marshall the map to JSON for safe output when used with `console.log()`.
Fixes #120
3a4f461 to
7a5de2f
Compare
7a5de2f to
886b852
Compare
886b852 to
a6d9371
Compare
a6d9371 to
152b56d
Compare
Previously we used to throw an error if we encountered a custom object as returned by CDP from an
Evaluate()orconsole.log()call.This should handle those cases properly now by converting objects to
map[string]interface{}internally, and using a custom Logrus formatter to marshall the map to JSON for safe output when used withconsole.log().Fixes #120