Expose the frame buffer and allow the user to specify options when creating a frame buffer. by tekkamanendless · Pull Request #118 · tebeka/selenium

tekkamanendless · 2018-05-02T16:17:30Z

I want to be able to save my runs as MP4 files, and that means that I need to know the X11 display number so I can pass it to avconv. I also want to be able to specify the screen size so that my tests can run against multiple resolutions.

This commit:

Exposes the frame buffer so that users can determine the display.
Allows you to pass options when creating a frame buffer.

The only option right now is for screen size, in particular so that you can set the color depth (the 8-bit default is no good for humans). Because the X11 display is exposed, you can write your own code to record from that display or take screenshots.

I created a new way to create a FrameBuffer called NewFrameBufferWithOptions, which accepts a map of options. I saw some todo items in there for doing other things; this options mechanism would allow for that without breaking the signature. I kept the old NewFrameBuffer for compatibility purposes, but it just calls the new one with no options.

Likewise, I created a new way to create a frame buffer ServiceOption called StartFrameBufferWithOptions, which accepts a map of options and passes them on to NewFrameBufferWithOptions. Again, to maintain compatibility, I kept StartFrameBuffer, which just calls the new one with no options.

This commit: 1. Exposes the frame buffer so that users can determine the display. 2. Allows you to pass options when creating a frame buffer. The only option right now is for screen size, in particular so that you can set the color depth (the 8-bit default is no good for humans). Because the X11 display is exposed, you can write your own code to record from that display or take screenshots.

codecov-io · 2018-05-02T16:20:34Z

Codecov Report

Merging #118 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #118      +/-   ##
=========================================
- Coverage    0.31%   0.31%   -0.01%     
=========================================
  Files           4       4              
  Lines         955     967      +12     
=========================================
  Hits            3       3              
- Misses        952     964      +12

Impacted Files	Coverage Δ
service.go	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a80d8b...190c159. Read the comment docs.

minusnine

Thanks for the PR!

I have one major comment, owing principally to the fact that it is important to make considered decisions

minusnine · 2018-05-02T16:52:13Z

service.go

 	// for which we provide a pipe.
-	xvfb := exec.Command("Xvfb", "-displayfd", "3", "-nolisten", "tcp")
+	arguments := []string{"-displayfd", "3", "-nolisten", "tcp"}
+	if value := options[FrameBufferOptionScreenSize]; len(value) > 0 {


Add a condition here to avoid inspecting options if it is nil.

minusnine · 2018-05-02T16:52:47Z

service.go

+// map.
 func StartFrameBuffer() ServiceOption {
+	options := map[string]string{}
+	return StartFrameBufferWithOptions(options)


Pass nil to the function instead of creating an empty map.

minusnine · 2018-05-02T16:55:36Z

service.go

+// This is equivalent to calling NewFrameBufferWithOptions with an empty map.
 func NewFrameBuffer() (*FrameBuffer, error) {
+	options := map[string]string{}
+	return NewFrameBufferWithOptions(options)


Pass nil to the function instead of creating an empty map.

minusnine · 2018-05-02T17:01:33Z

service.go

+
+// NewFrameBufferWithOptions starts an X virtual frame buffer running in the background.
+// You may pass in additional options (the "FrameBufferOption*" constants) to change its behavior.
+func NewFrameBufferWithOptions(options map[string]string) (*FrameBuffer, error) {


One worry I have about accepting a map[string]string is that it is very generic and we presently support only a single value for a key, which is not obvious to the caller.

In general, I usually like an API to be specific, flexible, or both: provide a native-language way to set specific, common options and optionally provide a facility for the caller to be more advanced. I like to be able to read the package's documentation and understand what common options are available and not have to resort to reading the documentation of the dependency too.

I would either:

Make this more specific: use a struct that specifically enumerates the options that can be set, or

Make this even more general: accept a slice of arbitrary arguments to be appended to the invocation, or

Only accept keys of a specific type and construct the FrameBufferOptionScreenSize value as this type. (I think I like this option the least), or

Use the functional option pattern akin to ServiceOption for Service (see also https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis). This provides consistency in this package.

I think I also dislike 2), as it can be accomplished by the caller managing a exec.Cmd instance outside of the API, and doesn't provide a good high-level API.

For 4), an invocation would look like:

fb, err := NewFrameBuffer(ScreenSize(1280, 1024, 8))

which looks nice.

What do you think?

I like #1, since #4 would need all kinds of chaining support for the different options.

minusnine · 2018-05-02T17:16:27Z

service.go

+// This is necessary in order to record or take screenshots from the frame buffer.
+func (s Service) FrameBuffer() *FrameBuffer {
+	return s.xvfb
+}


Out of curiosity:

I'm guessing you want to take a screenshot of more than just the browser, as provided by (WebDriver).Screenshot?

Once you have the *FrameBuffer, how will you take a screenshot? Is it worth adding a Screenshot function to the FrameBuffer type?

I'm using X11 stuff directly, so right now I'm using avconv to record video from that display. If WebDriver already has a screenshot mechanism, then I can use that, but it doesn't solve my video problem.

minusnine · 2018-05-02T17:18:56Z

service.go

 	output io.Writer
 }

+// FrameBuffer returns the FrameBuffer pointer.


Write:

// FrameBuffer returns the FrameBuffer if one was started by the service and nil otherwise.

or similar.

(It is important to point out that the poitner may be nil).

minusnine · 2018-05-02T17:19:36Z

service.go


+// FrameBuffer returns the FrameBuffer pointer.
+// This is necessary in order to record or take screenshots from the frame buffer.
+func (s Service) FrameBuffer() *FrameBuffer {


Should we just export the xvfb member from the struct?

Possibly? I only just started using this package a week or so ago, so whatever you guys think is best.

tekkamanendless · 2018-05-02T18:51:33Z

Okay, I also added some halfway-decent test coverage for the new stuff. In particular, the FrameBuffer stuff makes system calls, so I created a mechanism to mock that so that we can verify that the new FrameBufferOptions stuff is generating the correct Xvfb commands.

The exec.Command mocking mechanism is based on Go's own unit tests for exec.Command.

minusnine · 2018-05-02T19:19:02Z

service.go

+
+// This function is syntactically identical to `exec.Command`, but we want to be
+// able to switch it out for a different version for unit testing.
+var newExecCommand = exec.Command


nit: move this closer to its first use.

minusnine · 2018-05-02T19:19:34Z

service.go


 // NewFrameBuffer starts an X virtual frame buffer running in the background.
+//
+// This is equivalent to calling NewFrameBufferWithOptions with an empty map.


nit: s/empty map/empty options/

minusnine · 2018-05-02T19:20:35Z

service.go

+}
+
+// NewFrameBufferWithOptions starts an X virtual frame buffer running in the background.
+// You may pass in additional options (the "FrameBufferOption*" constants) to change its behavior.


nit: For at least this package, avoid the second person in technical documentation.

Also, this needs an update for the new function signature.

minusnine · 2018-05-02T19:21:28Z

service.go

 	// for which we provide a pipe.
-	xvfb := exec.Command("Xvfb", "-displayfd", "3", "-nolisten", "tcp")
+	arguments := []string{"-displayfd", "3", "-nolisten", "tcp"}
+	if len(options.ScreenSize) > 0 {


nit: since options.ScreenSize is a string, compare it to an empty string.

minusnine · 2018-05-02T19:23:58Z

service.go

+	// ScreenSize is the option for the frame buffer screen size.
+	// This is of the form "{width}x{height}[x{depth}]".  For example: "1024x768x24"
+	ScreenSize string
+}


Move this to just above StartFrameBufferWithOptions.

minusnine · 2018-05-02T19:32:03Z

service.go

 	// for which we provide a pipe.
-	xvfb := exec.Command("Xvfb", "-displayfd", "3", "-nolisten", "tcp")
+	arguments := []string{"-displayfd", "3", "-nolisten", "tcp"}
+	if len(options.ScreenSize) > 0 {


optional: Add validation that this string is of the form we expect.

I think a regular expression would be fine, something like:

if options.ScreenSize == "" { if !screenSizeRE.MatchString(`\d+x\d+(?:x\d+)`, options.ScreenSize) { return fmt.Errorf("invalid screen size: expected 'WxH[xD]', got %q", options.ScreenSize) } }

I suppose we could split and parse the string to ensure the digits aren't 0, but that's a lot of lines of code for a small gain.

minusnine · 2018-05-02T19:34:14Z

service_test.go

+		if frameBuffer.Display != "1" {
+			t.Errorf("frameBuffer.Display = %s, want %s", frameBuffer.Display, "1")
+		}
+		args := frameBuffer.cmd.Args[3:]


Use cmp.Diff instead of comparing individual elements. It is a lot less code.

https://godoc.org/github.com/google/go-cmp/cmp#Diff

Same above.

Still, this seems like a change detector test and isn't that useful:

https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html

I would instead do something like:

Start a FrameBuffer with a particular, non-default size and/or depth.

Interrogate the display using

d, err := xgbutil.NewConnDisplay(frameBuffer.Display) if err != nil { ... } defer d.Conn().Close() s := d.Screen() if s.WidthInPixels != 1024 { t.Errorf(...) } if s.HeightInPixels != 786 { t.Errorf(...) } ...

https://godoc.org/github.com/BurntSushi/xgbutil

The point of the test is that we don't actually have a frame buffer. I don't know what your setup is on the Github side, but I would assume that "Xvfb" and friends are not installed. If you know that they're present and working, I can make an integration test that validates that what we asked for is what we got.

I'll switch to cmp.Diff. I usually use assert.Equal and friends, but I didn't want to mess with the dependencies of this package.

I don't know what your setup is on the Github side, but I would assume that "Xvfb" and friends are not installed.

The tests run on Travis and make heavy use of Xvfb, e.g.

https://travis-ci.org/tebeka/selenium/builds/266849658

The main test suite is apparently silently broken and I've opened #119:

https://travis-ci.org/tebeka/selenium/builds/373720652

Xvfb should still be available though.

I'll switch to cmp.Diff. I usually use assert.Equal and friends, but I didn't want to mess with the dependencies of this package.

Thanks. I'm fine with adding dependencies especially for better testing. (Though I don't like the assert package in particular).

minusnine · 2018-05-03T17:13:06Z

Thanks again!

tekkamanendless · 2018-05-03T19:45:44Z

Haha, I was just about to fix the unit tests. Instead, see #121 :)

minusnine requested changes May 2, 2018

View reviewed changes

tekkamanendless added 2 commits May 2, 2018 13:56

Fixes for minusnine

1c3f7ef

Unit testing!

552f5dc

minusnine requested changes May 2, 2018

View reviewed changes

More fixes

190c159

minusnine approved these changes May 3, 2018

View reviewed changes

minusnine merged commit a49cf4b into tebeka:master May 3, 2018

Conversation

tekkamanendless commented May 2, 2018

Uh oh!

codecov-io commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

minusnine left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tekkamanendless commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minusnine commented May 3, 2018

Uh oh!

tekkamanendless commented May 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented May 2, 2018 •

edited

Loading