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

Extension of issue 244: Bypass spawning local browser #433

Closed
ken-p opened this Issue Dec 2, 2016 · 11 comments

Comments

Projects
None yet
2 participants
@ken-p

ken-p commented Dec 2, 2016

Hi,

As part of issue #244, a feature was added to not re-spawn a browser if it’s disappeared. The pref is in Configuration: selenide.reopenBrowserOnFail.

This is great, but we have one last use case :). In WebDriverThreadLocalContainer, method getWebDriver does not have that check:

log.info("No webdriver is bound to current thread: " + currentThread().getId() + " - let's create new webdriver");
return setWebDriver(createDriver());

We manage our own browser lifecycles (creating our own webdriver instance and using setWebDriver() in a superclass that TestNG tests inherit from) which includes running 20 threads with different browser configurations on a selenium grid. So we’d prefer for the test to fail if a browser goes unresponsive, or there’s a network problem talking to the hub, or the grid crashed, or we failed to properly call setWebDriver() in our own framework. Etc.

In other words, if no webdriver is bound to the current thread, we’d like an option to /not/ spawn a local FF browser. Perhaps throw an exception, like UnsupportedOperationException? Perhaps:

  @Override
  public WebDriver getWebDriver() {
    WebDriver webDriver = THREAD_WEB_DRIVER.get(currentThread().getId());
    if (webDriver != null) {
      return webDriver;
    }
    
    if (!reopenBrowserOnFail) {
       throw new UnsupportedOperationException("No webdriver is bound to current thread and pref 'reopenBrowserOnFail' is false.");
    }
    
    log.info("No webdriver is bound to current thread: " + currentThread().getId() + " - let's create new webdriver");
    return setWebDriver(createDriver());
  }

Or perhaps just return null instead? I'm open to any ideas!

Thanks!!

Ken

asolntsev added a commit that referenced this issue Dec 10, 2016

@asolntsev asolntsev self-assigned this Dec 10, 2016

@asolntsev asolntsev added this to the 4.2 milestone Dec 10, 2016

@asolntsev

This comment has been minimized.

Show comment
Hide comment
@asolntsev

asolntsev Dec 10, 2016

Member

@ken-p You are right, we have this problem.
I hope I have fixed the problem in Selenide 4.2-SNAPSHOT. Can you check the snapshot version somehow?

Member

asolntsev commented Dec 10, 2016

@ken-p You are right, we have this problem.
I hope I have fixed the problem in Selenide 4.2-SNAPSHOT. Can you check the snapshot version somehow?

@asolntsev asolntsev closed this Dec 10, 2016

@ken-p

This comment has been minimized.

Show comment
Hide comment
@ken-p

ken-p Dec 12, 2016

Sure, thanks! Is this the right place to grab it from, and which .jar should I use? https://oss.sonatype.org/content/repositories/snapshots/com/codeborne/selenide/4.2-SNAPSHOT/

ken-p commented Dec 12, 2016

Sure, thanks! Is this the right place to grab it from, and which .jar should I use? https://oss.sonatype.org/content/repositories/snapshots/com/codeborne/selenide/4.2-SNAPSHOT/

@asolntsev

This comment has been minimized.

Show comment
Hide comment
@asolntsev

asolntsev Dec 12, 2016

Member
Member

asolntsev commented Dec 12, 2016

@ken-p

This comment has been minimized.

Show comment
Hide comment
@ken-p

ken-p Dec 12, 2016

Thanks. Was the change made in WebDriverThreadLocalContainer? I'm looking in selenide-4.2-20161210.230300-5-sources and I don't see anything different...

ken-p commented Dec 12, 2016

Thanks. Was the change made in WebDriverThreadLocalContainer? I'm looking in selenide-4.2-20161210.230300-5-sources and I don't see anything different...

@asolntsev

This comment has been minimized.

Show comment
Hide comment
@asolntsev

asolntsev Dec 13, 2016

Member

Yes, the change was done in WebDriverThreadLocalContainer.getAndCheckWebDriver():

553a26d

Member

asolntsev commented Dec 13, 2016

Yes, the change was done in WebDriverThreadLocalContainer.getAndCheckWebDriver():

553a26d

@ken-p

This comment has been minimized.

Show comment
Hide comment
@ken-p

ken-p Dec 13, 2016

Oh, now I see it. Does a similar change need to be done to WebDriverThreadLocalContainer.getWebDriver() ?

ken-p commented Dec 13, 2016

Oh, now I see it. Does a similar change need to be done to WebDriverThreadLocalContainer.getWebDriver() ?

@ken-p

This comment has been minimized.

Show comment
Hide comment
@ken-p

ken-p Dec 13, 2016

Here's the simple test I was trying:

`import static com.codeborne.selenide.Selenide.$;

public class NoBrowser {

@Test
public void test() {
    //for this test we're assuming there's not a browser already on the thread local container
    System.setProperty("selenide.reopenBrowserOnFail", "false");
    $("blah").click();
}

}`

The $() resolves (I think) to WebDriverThreadLocalContainer.getWebDriver() (via WebDriverRunner). So how should that behave? As I said I'm happy with any behavior (exception, return null, etc).

Thanks again!

ken-p commented Dec 13, 2016

Here's the simple test I was trying:

`import static com.codeborne.selenide.Selenide.$;

public class NoBrowser {

@Test
public void test() {
    //for this test we're assuming there's not a browser already on the thread local container
    System.setProperty("selenide.reopenBrowserOnFail", "false");
    $("blah").click();
}

}`

The $() resolves (I think) to WebDriverThreadLocalContainer.getWebDriver() (via WebDriverRunner). So how should that behave? As I said I'm happy with any behavior (exception, return null, etc).

Thanks again!

@asolntsev

This comment has been minimized.

Show comment
Hide comment
@asolntsev

asolntsev Dec 13, 2016

Member

@ken-p Hi!
Setting name sound like "...OnFail". My idea was that it should open browser for the first time, but avoid re-opening it after a failure.

So, method $("blah").click(); should open a new browser.

But the following code:

$("blah").click();  // opens a browser
// say, browser is unexpectedly closed by the Matrix
$("blah").click();  // does NOT re-open the browser
Member

asolntsev commented Dec 13, 2016

@ken-p Hi!
Setting name sound like "...OnFail". My idea was that it should open browser for the first time, but avoid re-opening it after a failure.

So, method $("blah").click(); should open a new browser.

But the following code:

$("blah").click();  // opens a browser
// say, browser is unexpectedly closed by the Matrix
$("blah").click();  // does NOT re-open the browser
@ken-p

This comment has been minimized.

Show comment
Hide comment
@ken-p

ken-p Dec 13, 2016

That makes perfect sense....except some of our edge cases :). As I said, we're managing our own browser lifecycles and storing them using WebDriverRunner.setWebDriver(). Over a 3 hour test run with 3k tests running through the CI system, it's inevitable that something goes wrong and there's no webdriver on the thread. If we are doing a test run (for example) to verify IE functionality, we'd really rather not have a FF browser spin up at all if one is not bound to the thread (and especially since we want that test to run on a particular VM configuration on a grid). If this sounds too edge case-y for your framework I can fork and manage myself. Thanks again! We've had tremendous success with Selenide :).

ken-p commented Dec 13, 2016

That makes perfect sense....except some of our edge cases :). As I said, we're managing our own browser lifecycles and storing them using WebDriverRunner.setWebDriver(). Over a 3 hour test run with 3k tests running through the CI system, it's inevitable that something goes wrong and there's no webdriver on the thread. If we are doing a test run (for example) to verify IE functionality, we'd really rather not have a FF browser spin up at all if one is not bound to the thread (and especially since we want that test to run on a particular VM configuration on a grid). If this sounds too edge case-y for your framework I can fork and manage myself. Thanks again! We've had tremendous success with Selenide :).

@asolntsev

This comment has been minimized.

Show comment
Hide comment
@asolntsev

asolntsev Dec 13, 2016

Member

@ken-p Yes, I think Selenide 4.2 works exactly as you described. Your problem should be solved now.

Member

asolntsev commented Dec 13, 2016

@ken-p Yes, I think Selenide 4.2 works exactly as you described. Your problem should be solved now.

@ken-p

This comment has been minimized.

Show comment
Hide comment
@ken-p

ken-p Dec 13, 2016

Thanks! I will let you know if I find a case that describes otherwise.

ken-p commented Dec 13, 2016

Thanks! I will let you know if I find a case that describes otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment