Add tests for BrowserContextOptions #65
Conversation
common/types.go
Outdated
| return ColorSchemeToString[c] | ||
| } | ||
|
|
||
| var ColorSchemeToString = map[ColorScheme]string{ |
There was a problem hiding this comment.
This can be unexported since it's only used in this package. Otherwise you'd have to add a comment to document it, or the linter should complain. Right now it doesn't appear to be setup properly (I'll add a note to #58 mentioning this as well).
But in general instead of maintaining our own conversion maps for these things, in k6 we use enumer which handles this for us, including the JSON marshalling. I can create an issue and do that in a follow-up PR if you don't have time right now.
There was a problem hiding this comment.
Ah, yeah the enumer lib seems exactly like what we need 😅 Yes please, I'd prefer if you could help with the move to enumer in a separate PR 🙏
| defer bt.Browser.Close() | ||
|
|
||
| t.Run("BrowserContextOptions", func(t *testing.T) { | ||
| t.Run("should have correct default values", func(t *testing.T) { testBrowserContextOptionsDefaultValues(t, bt) }) |
There was a problem hiding this comment.
I've noticed you use this pattern of using a separate testing function. Why not just have it inline here? I think that would be more readable instead of having to go to the function, which in a large file is a chore (if you don't have go-to-definition setup).
There was a problem hiding this comment.
I guess just a preference to have sort of TOC of the tests in a file (and yes I rely on go-to-definition), not a strongly held preference by any means and you and Inanc should enforce whatever style you prefer :)
Perfhaps we can change all the tests in a separate PR?
There was a problem hiding this comment.
Yep, I created an issue for doing that: #52
We can restructure the extension after ObsCon 🤞
robingustafsson
force-pushed
the
tests/add-browser-context-options-tests
branch
from
7e4c239 to
fefec9a
Compare
|
Seems like gh is having some issues, seeing 502 gateway errors pulling some deps in tests 🤷♂️ Tests are passing on Linux and locally on my Mac though so I'll merge to main anyways :) |
No description provided.