-
-
Notifications
You must be signed in to change notification settings - Fork 322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix nil pointer in screenshot #632
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the CI error: https://github.com/yusufozturk/rod/runs/6994899014?check_suite_focus=true
I cant repro the issue for tests at the moment. If I can in the future, I can reopen this PR. If anybody has same issue, they can see the PR details. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use code like this in the unit test to mock the nil values:
Lines 627 to 632 in fe0a731
g.mc.stub(1, proto.PageGetResourceContent{}, func(send StubSend) (gson.JSON, error) { | |
return gson.New(proto.PageGetResourceContentResult{ | |
Content: "ok", | |
Base64Encoded: false, | |
}), nil | |
}) |
I'm having a stab at this but I'm running into issues in the test. func TestPageScreenshot(t *testing.T) {
g := setup(t)
f := filepath.Join("tmp", "screenshots", g.RandStr(16)+".png")
p := g.page.MustNavigate(g.srcFile("fixtures/click.html"))
p.MustElement("button")
p.MustScreenshot()
data := p.MustScreenshot(f)
img, err := png.Decode(bytes.NewBuffer(data))
g.E(err)
g.Eq(1280, img.Bounds().Dx())
g.Eq(800, img.Bounds().Dy())
g.Nil(os.Stat(f))
p.MustScreenshot("")
g.Panic(func() {
g.mc.stubErr(1, proto.PageCaptureScreenshot{})
p.MustScreenshot()
})
g.Panic(func() {
g.mc.stub(1, proto.PageGetLayoutMetrics{}, func(send StubSend) (gson.JSON, error) {
return gson.New(proto.PageGetLayoutMetricsResult{}), nil
})
p.MustScreenshot()
})
} When executing
The code under test is: func (p *Page) Screenshot(fullpage bool, req *proto.PageCaptureScreenshot) ([]byte, error) {
if req == nil {
req = &proto.PageCaptureScreenshot{}
}
if fullpage {
metrics, err := proto.PageGetLayoutMetrics{}.Call(p)
if err != nil {
return nil, err
}
if metrics.CSSContentSize == nil {
return nil, errors.New("failed to get css content size")
}
oldView := proto.EmulationSetDeviceMetricsOverride{}
set := p.LoadState(&oldView)
view := oldView
view.Width = int(metrics.CSSContentSize.Width)
view.Height = int(metrics.CSSContentSize.Height)
err = p.SetViewport(&view)
if err != nil {
return nil, err
}
defer func() { // try to recover the viewport
if !set {
_ = proto.EmulationClearDeviceMetricsOverride{}.Call(p)
return
}
_ = p.SetViewport(&oldView)
}()
}
shot, err := req.Call(p)
if err != nil {
return nil, err
}
return shot.Data, nil
} What am I doing wrong? |
@alexferrari88 because |
Ups sorry, I clicked a wrong button while reading your comment @ysmood I was in holiday and I can get back on this. |
this can be closed because of #699 |
This PR simply adds some additional nil checks to prevent panics in screenshot.
Fixes #631
Development guide
Link
Test on local before making the PR