Skip to content

Added new attributes for automation_session object and updated endpoint #2

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hanikhan
Copy link

Added browser_console_logs_url, har_logs_url, appium_logs_url to the json returned for session details. Also updated REST API endpoint to reflect latest BrowserStack API endpoint: https://www.browserstack.com/automate/rest-api

…json returned for session details. Also updated REST API endpoint to reflect latest BrowserStack API endpoint: https://www.browserstack.com/automate/rest-api
* @return The browser_console_logs_url
*/
@JsonProperty("browser_console_logs_url")
public String getBrowser_console_logs_url() {
Copy link

@raghuhit raghuhit Aug 7, 2018

Choose a reason for hiding this comment

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

Use camel case only in all function names.

Copy link

@raghuhit raghuhit left a comment

Choose a reason for hiding this comment

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

  1. Please make all variable and function names to be camelcased.
  2. Error messages should be in accordance with the function they are being written in, for example, in getConsoleLogs function, error message should be "Session console logs not found."

throw new AutomateException("Invalid session", 400);
}

if (session.getBrowser_console_logs_url() == null) {
Copy link

Choose a reason for hiding this comment

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

This should also check whether it is a zero length string or not.

throw new AutomateException("Invalid session", 400);
}

if (session.getHar_logs_url() == null) {
Copy link

Choose a reason for hiding this comment

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

Add check for zero length string too.
Also this check is not needed, since you are doing it already in Session.java

}

if (session.getBrowser_console_logs_url() == null) {
throw new AutomateException("Session logs not found", 404);
Copy link

Choose a reason for hiding this comment

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

Error message should be "Session console logs not found."

}

if (session.getHar_logs_url() == null) {
throw new AutomateException("Session logs not found", 404);
Copy link

Choose a reason for hiding this comment

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

Error message should be "Session HAR logs not found."

throw new AutomateException("Invalid session", 400);
}

if (session.getAppium_logs_url() == null) {
Copy link

Choose a reason for hiding this comment

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

Add check for zero length string too.

}

public final String getHARLogs() throws AutomateException {
if (logUrl == null) {
Copy link

Choose a reason for hiding this comment

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

  1. Check should be on harLogs variable.

private String har_logs_url;

@JsonProperty("appium_logs_url")
private String appium_logs_url;
Copy link

Choose a reason for hiding this comment

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

variables name should be in camel case.

}

public final String getAppiumLogs() throws AutomateException {
if (logUrl == null) {
Copy link

Choose a reason for hiding this comment

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

variable name is not correct and zero length string check needs to be performed also here,


@Test
public void testGetSessionHARLogs() {
// TODO: Verify if logs are non-empty
Copy link

Choose a reason for hiding this comment

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

This test will fail sometimes. If the last run session haven't enabled network logs.

}

@Test
public void testGetSessionAppiumLogs() {
Copy link

Choose a reason for hiding this comment

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

This test will fail in case the last session is run on desktop or appium logs capability was not passed in that.

@hanikhan
Copy link
Author

hanikhan commented Aug 7, 2018

Added requested changes. Please review

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants