some suggestions

[l3pp4rd]

Hi, looks nice, I referenced it in godog readme.

I also suggest to have a look at some contexts which are already well thought, like one from behat The best is to learn from something, which is already there, for more than few years.

Steps like this are false positive, because sometimes javascript creates the DOM and it takes time to load. Such assertions which says I should not see are not a good idea, because the question is for how long I should not see this, so I could believe it is not there, and having tests which waits 5 seconds to assert that something is not visible - is a bad practice. So you only should have something which says I should see and in the code usually, it is polling the DOM for a selector with a timeout which results in assertion failure. NOTE: all this is because of javascript, without it, HTML is rendered when the page loads and that is it.

Why I'm raising this related to js, because in most cases people now use Javascript in their pages, in fact they only use selenium to test javascript related rendering. Having such delayed html rendering, makes it hard for users to understand, what exactly is wrong and why my step I should see sometimes fails. Because Selenium as far as I remember, does not (and cannot wait for javascript to load, since you cannot know how long it will take).

Also, methods like this are just proxies, you can simply register them as a step, without wrapping in a step func.

Taking screenshot after failed scenario will fail on windows, use filepath.Join and though, it would be great to manage screenshots manually. Could instead show an example on Readme. Because it would be hard to fit all user wishes, custom directories, maybe sending an error report by email with a screenshot and etc..

Now more general stuff

When users register these with NewBrowserSteps it always, before every scenario will open a browser session. Which is a bad idea, because usually in suite, there can be scenarios, which are not running on web.

For example Behat adds a @javascript tag, if there is this tag on any scenario, it runs a Selenium session then. And this makes sense, because starting selenium session, is an expensive and time consuming task.

Also NewBrowserSteps cannot have an error, it returns always nil as an error.

In overall, that looks useful. Just not sure yet, weather it is simple enough to use selenium or phantomjs directly and create such context on user specific needs. Because some of the abstractions here are leaking, like screenshot for example.

[llonchj]

Thanks for your comments @l3pp4rd, I really appreciate them.

I totally understand what you mention with the i should see, will be a good practice to implement a generic timeout while "i do not see" the element. That will make the tests more stable. Possibly the timeout value should be a optional argument for the step "i should see X" with default timeout, and "i should see X before 2 seconds"

When you mention that some steps are proxies (i.e. direct functions: reload, back, etc..) how can I get the WebDriver at buildSteps definition as the WebDriver is created Before the Scenario?

In relation to selenium session creation, will be a good idea to identify if any of the steps in the scenario requires browser automation instead of a @javascript?

[l3pp4rd]

Hi, what I have meant with proxies, is that a method like this can be removed. And instead directly in step definition it can reference that navigation function, since it has the same interface as a step:

instead of

s.Step(`^I navigate forward$`, b.iNavigateForward)

make it:

s.Step(`^I navigate forward$`, b.GetWebDriver().Forward())

Some of the tests may not need javascript support, and instead they can just use a http.Client. If for example you need jjavascript, you tag a scenario with @javascript and based on that tag, your context BeforeScenario gets the selenium connection. here you need to check the tag. Otherwise it could error if tag is not enabled and other steps are used, or instead it could use http.Client for some of the steps. That may require a little more work to do it properly, but it could make sense to improve performance, because opening a selenium connection when it is not necessary is quite an expensive operation.

[llonchj]

Thanks @l3pp4rd, Unless i'm missing something the proxy clearly is not working because GetWebDriver is null until BeforeScenario is ran.

Something like this works:

s.Step(`^I navigate forward$`, func() error { return b.GetWebDriver().Forward()})

[l3pp4rd]

ah, OK, logical mistake then ;)

[llonchj]

@l3pp4rd I pushed the proxy fix (8a84609b4dd53b1babcd8ccfac0a0e88391a10bc), added SCREENSHOT_PATH environment variable (c431d47fb40407d6d096a1af6dfa18aba3cf3a21) and removed error in NewBrowserSteps(29388b63349c5e54e69878ce3815ddcf022c1bb1).

I will think about a strategy for the timeout in steps like "I should see...".

What will be better: a @javascript tag or determine in BeforeScenario if one of the steps requires selenium and instantiate?

I really appreciate 👍 your comments! Thank you

[l3pp4rd]

I think it is better to have a tag, since that allows to run godog --tags=@javascript or exclude such tag scenarios, which would run other tests faster.