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.

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

JsModifier - Running JavaScript command as in the browser console. #69

Merged
merged 3 commits into from
Apr 12, 2017

Conversation

pandlab
Copy link
Contributor

@pandlab pandlab commented Mar 2, 2017

JsModifier - Running JavaScript command as in the browser console.

@subiron
Copy link
Contributor

subiron commented Mar 2, 2017

Hi @Pnad
Thank you for contribution, however couple of things are missing in your pull request, please read CONTRIBUTING.md page.
Especially please provide sample test suite with usage of this modifier (see test-suite module), update documentation and functional tests.

.. going forward please consider making "JavaScript Collector"... which would gather output of executed JavaScript Command and then 'source comparator' could be used to compare that output.

@Override
public CollectorStepResult collect() throws ProcessingException {
if (LOG.isDebugEnabled()) {
LOG.debug("Execute JS cmd: {} on page: {} properties url: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

properties.getUrl() x2 ?

((JavascriptExecutor) driver).executeScript(cmd);

result = CollectorStepResult.newModifierResult();
} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to always return result even if something went wrong we want to have this information on report not just in logs.
please return newProcessingErrorResult as a result with message from WebDriverException (catch it first)


result = CollectorStepResult.newModifierResult();
} catch (Exception e) {
throw new ProcessingException("Can't execute cmd = " + cmd, e);
Copy link
Contributor

Choose a reason for hiding this comment

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

consider changing 'cmd' to something more meaningful e.g. 'Can't execute JavaScript command'

Copy link
Contributor

@malaskowski malaskowski left a comment

Choose a reason for hiding this comment

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

Hello @Pnad !
Thank you for your contribution.
I've added some comments in this Pull Request.

import java.util.List;
import java.util.Map;

public class JsModifier implements CollectorJob {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check formatting in this class - AET project follows those coding conventions. Please apply google styleguide.


public class JsModifier implements CollectorJob {

public static final String NAME = "js";
Copy link
Contributor

Choose a reason for hiding this comment

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

Modifier named js is not very descriptive. How about renaming it to execute-js?

Copy link
Contributor

Choose a reason for hiding this comment

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

imho shorter is better in this case -this will be used as a tag in xml suite and probably 'cmd' value will be long as hell ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't agree. We already have js-errors collector. It may be misleading having js tag.
I suggest renaming cmd param to script also.

About long script - I thought about this some time ago and there is just an idea, how about providing script as a web resource and script value should be just a link for it?
E.g.

<execute-js script="http://my-site-to-test/aet/some-script.js" />

This solution has its disadvantages, however it will not obscure the suite XML definition.

@@ -0,0 +1,88 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a documentation for this modifier. The instruction how to do this can be found in our contributing rules.
You should create a new page in documentation module.

ParametersValidator.checkNotBlank(cmd, "cmd parameter is mandatory");
}

public CollectorStepResult jsElement(WebDriver driver, String cmd) throws ProcessingException {
Copy link
Contributor

Choose a reason for hiding this comment

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

please change method name to e.g. ''executeJavaScript"

@malaskowski
Copy link
Contributor

Hello @Pnad,
Please let us know if you are going to finish this modifier. If not I'm going to close this Pull Request as unfinished.
Thank you

@pandlab
Copy link
Contributor Author

pandlab commented Mar 17, 2017

@Skejven Yes, I'm going.

…ser console.

___
I hereby agree to the terms of the AET Contributor License Agreement.
Copy link
Contributor Author

@pandlab pandlab left a comment

Choose a reason for hiding this comment

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

Please, Check the current version.
The changes I made on the basis of the existing code (HideModifer).

Copy link
Contributor

@malaskowski malaskowski left a comment

Choose a reason for hiding this comment

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

Thank you @Pnad for fixes.
The last thing missing are integration tests.
Please see tests that existing modules already have:
https://github.com/Cognifide/aet/tree/master/integration-tests/test-suite/partials
And try add one for this modifier.
E.g. change some random value to constant one with js script before taking screenshot, so the test will pass.

Example of the test could look like this:

<open />
<sleep duration="1000" />
<executejavascript cmd"..."/>
<screen />


import java.util.Map;

import static org.mockito.Mockito.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use * imports.

}

@Test
public void ExecuteJavaScriptModifier_HideElements()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure... What this test proves exactly?

@pandlab
Copy link
Contributor Author

pandlab commented Mar 24, 2017

I don't have much experience in AET and java. I created the code analogously to AET code.
I added to integration test. Check it, please.

@malaskowski malaskowski changed the base branch from master to feature/js-command-modifier April 12, 2017 09:36
@malaskowski malaskowski merged commit 6cb2a5d into wttech:feature/js-command-modifier Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants