Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update to selenium 3.14.0. #420

Open
wants to merge 4 commits into
base: 2.x
Choose a base branch
from
Open

Update to selenium 3.14.0. #420

wants to merge 4 commits into from

Conversation

damencho
Copy link

Contains some changes that are covered in #402
Tests are passing.

@damencho
Copy link
Author

damencho commented Aug 16, 2018 •
edited
Loading

@shankarkc Can you now check this. This is using branch 2.x as a base.

@shankarkc
Copy link
Contributor

shankarkc commented Aug 16, 2018 •
edited
Loading

@damencho Thanks a lot for quick turnaround time. I will check tomorrow as its night here IST. I have noticed that com.groupon.seleniumgridextras.grid.proxies.SetupTeardownProxy was broken in selenium grid extra due to changes in selenium grid code base. When I used SetupTeardownProxy it didnt work for me. When i removed it it worked. With the new build does nodes getting registered with hub fine? I was experiencing 500 error last time.

shankarkc
shankarkc reviewed Aug 16, 2018
View reviewed changes
...GridExtras/src/main/java/com/groupon/seleniumgridextras/grid/proxies/ExtrasHtmlRenderer.java
@@ -73,7 +71,6 @@ public String renderSummary() {
}

private String getIcon(Map<String, Object> capabilities) {
return BrowserNameUtils.getConsoleIconPath(new DesiredCapabilities(capabilities),
proxy.getRegistry());
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might not be right. Depending on the browser it should return icon for the browser.

	  public static String getConsoleIconPath(DesiredCapabilities cap, GridRegistry registry) {
	    String name = consoleIconName(cap, registry);
	    String path = "org/openqa/grid/images/";
	    InputStream in =
	        Thread.currentThread().getContextClassLoader()
	            .getResourceAsStream(path + name + ".png");
	    if (in == null) {
	      return null;
	    }
	    return "/grid/resources/" + path + name + ".png";
	  }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, maybe you are right. But this is just how we run, not sure where this is used ... It was the quickest way to get rid of BrowserNameUtils.
I can update the code if you want.
In my changes, there was some dead code that I removed, rather than updating it, like a servlet in this PR.

Copy link
Contributor

@shankarkc shankarkc Aug 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its used in grid console page...I am not sure though. I looked at old code and copy pasting it here so that it helps you to fix it quickly

 private static String consoleIconName(DesiredCapabilities cap, GridRegistry registry) {
	    String browserString = cap.getBrowserName();
	    if (browserString == null || "".equals(browserString)) {
	      return "missingBrowserName";
	    }

	    String ret = browserString;

	    // Map browser environments to icon names.
	    if (browserString.contains("iexplore") || browserString.startsWith("*iehta")) {
	      ret = BrowserType.IE;
	    } else if (browserString.contains("firefox") || browserString.startsWith("*chrome")) {
	      if (cap.getVersion() != null && cap.getVersion().toLowerCase().equals("beta") ||
	          cap.getBrowserName().toLowerCase().contains("beta")) {
	        ret = "firefoxbeta";
	      } else if (cap.getVersion() != null && cap.getVersion().toLowerCase().equals("aurora") ||
	          cap.getBrowserName().toLowerCase().contains("aurora")) {
	        ret = "aurora";
	      } else if (cap.getVersion() != null && cap.getVersion().toLowerCase().equals("nightly") ||
	          cap.getBrowserName().toLowerCase().contains("nightly")) {
	        ret = "nightly";
	      } else {
	        ret = BrowserType.FIREFOX;
	      }

	    } else if (browserString.startsWith("*safari")) {
	      ret = BrowserType.SAFARI;
	    } else if (browserString.startsWith("*googlechrome")) {
	      ret = BrowserType.CHROME;
	    } else if (browserString.toLowerCase().contains("edge")) {
	      ret = BrowserType.EDGE;
	    }

	    return ret.replace(" ", "_");
	  }


	  /**
	   * get the icon representing the browser for the grid. If the icon cannot be located, returns
	   * null.
	   *
	   * @param cap - Capability
	   * @param registry - Registry
	   * @return String with path to icon image file.  Can be null if no icon
	   *         file if available.
	   */
	  public static String getConsoleIconPath(DesiredCapabilities cap, GridRegistry registry) {
	    String name = consoleIconName(cap, registry);
	    String path = "org/openqa/grid/images/";
	    InputStream in =
	        Thread.currentThread().getContextClassLoader()
	            .getResourceAsStream(path + name + ".png");
	    if (in == null) {
	      return null;
	    }
	    return "/grid/resources/" + path + name + ".png";
	  }

@damencho
Copy link
Author

Yep, SetupTeardownProxy was broken in master when using latest selenium. It is fixed in 2.x branch, the one we are using.
My initiative here was that we are using some changes to run our selenium grid using selenium-extras and I just wanted to push those modifications for the community, and I just took the diffs to create patches, but apparently, I used the wrong branch :)

shankarkc
shankarkc suggested changes Aug 16, 2018
View reviewed changes
Copy link
Contributor

@shankarkc shankarkc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls take a look at getIcon method below. I think it needs little modification

@shankarkc
Copy link
Contributor

@damencho please let me know the git repo/branch that I can use. I will test it and let you know the results tomorrow.

@damencho
Copy link
Author

It seems to me it is not used, at least I don't see a broken icon or anything on the UI.
This is the repo and the branch: https://github.com/damencho/Selenium-Grid-Extras/tree/update-selenium-2.x

@shankarkc
Copy link
Contributor

shankarkc commented Aug 17, 2018 via email

@damencho
Copy link
Author

I see, while fixing the tests I saw that, but didn't think it will be an issue. The Gson serializer is supposed to be removing those by default ... I will check these days or next week, when I have time.

@shankarkc
Copy link
Contributor

shankarkc commented Aug 17, 2018 •
edited
Loading

@damencho same issue is with browserTimeout too.

Once its fixed I do see grid node getting registred to hub

Great work. 👏 🙇 ✨✨

@shankarkc
Copy link
Contributor

@damencho I will run some UI tests on this grid and update the thread if all is good.

@shankarkc
Copy link
Contributor

shankarkc commented Aug 17, 2018 •
edited
Loading

@damencho selenium grid extra hub URL is not working ie on hub node if i run http://10.xxx.xxx.1:3000 it should open video page. But it times out. i see this error

Navigate to http://10.100.192.1:3000 for more details
[Thread-14] ERROR com.xuggle.ferry.JNILibraryLoader - Could not load library: xuggle; version: 5; Visit http://www.xuggle.com/xuggler/faq/ to find common solutions to this problem
Exception in thread "Thread-14" java.lang.UnsatisfiedLinkError: no xuggle in java.library.path
        at java.lang.ClassLoader.loadLibrary(Unknown Source)
        at java.lang.Runtime.loadLibrary0(Unknown Source)
        at java.lang.System.loadLibrary(Unknown Source)
        at com.xuggle.ferry.JNILibraryLoader.loadLibrary0(JNILibraryLoader.java:268)
        at com.xuggle.ferry.JNILibraryLoader.loadLibrary(JNILibraryLoader.java:171)
        at com.xuggle.ferry.JNILibrary.load(JNILibrary.java:161)
        at com.xuggle.ferry.FerryJNI.<clinit>(FerryJNI.java:16)
        at com.xuggle.ferry.Ferry.<clinit>(Ferry.java:25)
        at com.xuggle.xuggler.XugglerJNI.<clinit>(XugglerJNI.java:19)
        at com.xuggle.xuggler.IRational.make(IRational.java:416)
        at com.groupon.seleniumgridextras.utilities.threads.video.VideoRecorderCallable.<clinit>(VideoRecorderCallable.java:43)

Also in node i dont see video getting recorded. Most likely bcz of above call stack.

@damencho
Adds some null checks.
8427414 
Fixes the resulting config file to not have values like:
"browserTimeout": "null" and
"timeout": "null"
@damencho
Copy link
Author

I fixed the browserTimeout and timeout.

The missing xuggle, I don't think it has anything to do with my PR, I think if you checkout just the 2.x branch you will have the same error or it is something local.
I will check our fork, cause it was running with xuggle for some time, so if there is anything around that ...

@damencho
Copy link
Author

Nope, I don't see anything related to that, so branch 2.x should be running fine.
And also in the target jar-with-dependencies all the native libraries are there, are you sure it is not something local on your deployment?

@shankarkc
Copy link
Contributor

shankarkc commented Aug 19, 2018 via email

@leogcba
Copy link

leogcba commented Oct 16, 2018

Hello guys! Is this project completely abandoned?

@leogcba leogcba mentioned this pull request Oct 16, 2018
Open
@manelpb
Copy link

manelpb commented Oct 18, 2018

I have the same question :(

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

@shankarkc shankarkc shankarkc requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@damencho @shankarkc @leogcba @manelpb