Standardize dynamic variables API #2376

valfirst · 2022-01-13T11:33:49Z

No description provided.

vividus-engine/src/main/java/org/vividus/steps/VariableResolver.java

ikalinin1 · 2022-01-13T12:40:05Z

vividus-engine/src/main/java/org/vividus/variable/DynamicVariable.java

-    String getValue();
+    Optional<String> calculateValue();
+
+    String getMissingValueErrorMessage();


I think this approach may limit the implementation when during variable resolution more than one error may appear, maybe we could return some pojo like

class DynamicVaruableValue { private final Optional<Object> value; private final String failDetails; }

do we have examples where the first error don't stop variable resolution process? so several errors can stack up

It not necessarily to stack them up. What if the resolution could resolve into different errors? How to process it with the current implementation. I'll look closely, but clipboard text resolution may fail by different reasons

ikalinin1 · 2022-01-13T12:42:44Z

...tension-selenium/src/main/java/org/vividus/ui/variable/AbstractWebDriverDynamicVariable.java

+    @Override
+    public String getMissingValueErrorMessage()
+    {
+        return "application is not started";


Suggested change

return "application is not started";

return "session is not started";

users don't understand what session is

codecov · 2022-01-13T13:32:30Z

Codecov Report

Merging #2376 (0165d01) into master (870cf69) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #2376   +/-   ##
=========================================
  Coverage     96.34%   96.35%           
- Complexity     5400     5411   +11     
=========================================
  Files           765      769    +4     
  Lines         15499    15532   +33     
  Branches       1032     1035    +3     
=========================================
+ Hits          14933    14966   +33     
  Misses          442      442           
  Partials        124      124

Impacted Files	Coverage Δ
.../main/java/org/vividus/steps/VariableResolver.java	`100.00% <100.00%> (ø)`
...dus/variable/DynamicVariableCalculationResult.java	`100.00% <100.00%> (ø)`
...AbstractSearchContextRectangleDynamicVariable.java	`100.00% <100.00%> (ø)`
.../ui/variable/AbstractWebDriverDynamicVariable.java	`100.00% <100.00%> (ø)`
...i/variable/SearchContextHeightDynamicVariable.java	`100.00% <100.00%> (ø)`
...ui/variable/SearchContextWidthDynamicVariable.java	`100.00% <100.00%> (ø)`
...iable/SearchContextXCoordinateDynamicVariable.java	`100.00% <100.00%> (ø)`
...iable/SearchContextYCoordinateDynamicVariable.java	`100.00% <100.00%> (ø)`
...vividus/ui/variable/SourceCodeDynamicVariable.java	`100.00% <100.00%> (ø)`
...bileapp/variable/ClipboardTextDynamicVariable.java	`100.00% <100.00%> (ø)`
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 870cf69...0165d01. Read the comment docs.

uarlouski · 2022-01-14T09:11:23Z

vividus-engine/src/main/java/org/vividus/variable/DynamicVariableCalculationResult.java

+        return new DynamicVariableCalculationResult(Optional.empty(), Optional.of(error));
+    }
+
+    public static DynamicVariableCalculationResult withValueAndError(Optional<String> value,


the "and" method seems a bit misleading for me because if the value is not empty the error is lost and never reachable in the code

please propose a new name

https://github.com/vividus-framework/vividus/pull/2376/files#r784737856

or maybe like a named constructor DynamicVariableCalculationResult.error(String error) DynamicVariableCalculationResult.value(String value)

in general I'm okay with describing this behaviour in javadoc and explaining that this method exists because we don't want to use if-else construction :)

uarlouski · 2022-01-14T09:17:02Z

vividus-engine/src/main/java/org/vividus/variable/DynamicVariableCalculationResult.java

+
+    public Optional<String> getValueOrHandleError(Consumer<String> errorHandler)
+    {
+        if (!value.isPresent())


value.isEmpty() is a bit more concise :)

ikalinin1 · 2022-01-14T10:37:29Z

docs/modules/developer-guides/pages/custom-plugins.adoc

+In order to create and register own dynamic variable the following steps should
+be done.
+
+. Create a new class that implements `org.vividus.variable.DynamicVariable`:


Maybe add a link to the class on GitHub?

btw can we have a crawler to ensure links in docs are not broken?

it worths to explore options:
https://gitlab.com/antora/xref-validator
https://github.com/owncloud/docs/blob/master/docs/checking-broken-links.md
https://github.com/timaschew/link-checker
https://github.com/Munter/netlify-plugin-checklinks

ikalinin1 · 2022-01-14T10:38:53Z

vividus-engine/src/main/java/org/vividus/variable/DynamicVariableCalculationResult.java

+    public static DynamicVariableCalculationResult withValue(String value)
+    {
+        return new DynamicVariableCalculationResult(Optional.of(value), Optional.empty());
+    }


Suggested change

public static DynamicVariableCalculationResult withValue(String value)

{

return new DynamicVariableCalculationResult(Optional.of(value), Optional.empty());

}

public static DynamicVariableCalculationResult of(String value)

{

return new DynamicVariableCalculationResult(Optional.of(value), Optional.empty());

}

it could be ambiguous

sonarqubecloud · 2022-01-14T13:03:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

valfirst marked this pull request as draft January 13, 2022 11:33

ikalinin1 reviewed Jan 13, 2022

View reviewed changes

vividus-engine/src/main/java/org/vividus/steps/VariableResolver.java Outdated Show resolved Hide resolved

ikalinin1 reviewed Jan 13, 2022

View reviewed changes

valfirst force-pushed the standardize-dynamic-variables-api branch from eef69ea to 56d8261 Compare January 13, 2022 13:08

valfirst force-pushed the standardize-dynamic-variables-api branch 3 times, most recently from 73c2f6a to d7d42cf Compare January 13, 2022 16:21

valfirst requested review from ikalinin1 and uarlouski January 13, 2022 16:21

valfirst force-pushed the standardize-dynamic-variables-api branch 5 times, most recently from 429e2df to 8e60db9 Compare January 14, 2022 08:36

valfirst marked this pull request as ready for review January 14, 2022 08:36

uarlouski reviewed Jan 14, 2022

View reviewed changes

valfirst force-pushed the standardize-dynamic-variables-api branch from 8e60db9 to fee259e Compare January 14, 2022 10:00

ikalinin1 reviewed Jan 14, 2022

View reviewed changes

ikalinin1 approved these changes Jan 14, 2022

View reviewed changes

uarlouski approved these changes Jan 14, 2022

View reviewed changes

Standardize dynamic variables API

0165d01

valfirst force-pushed the standardize-dynamic-variables-api branch from fee259e to 0165d01 Compare January 14, 2022 12:43

valfirst requested a review from uarlouski January 14, 2022 12:45

uarlouski approved these changes Jan 14, 2022

View reviewed changes

valfirst mentioned this pull request May 14, 2022

Introduce link checker vividus-framework/docs.vividus.dev#24

Open

valfirst merged commit 0a3bedb into master Jan 14, 2022

valfirst deleted the standardize-dynamic-variables-api branch January 14, 2022 13:43

VitaliaPiliuhina pushed a commit to VitaliaPiliuhina/vividus that referenced this pull request Apr 8, 2022

Standardize dynamic variables API (vividus-framework#2376)

d62d262

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize dynamic variables API #2376

Standardize dynamic variables API #2376

valfirst commented Jan 13, 2022

ikalinin1 Jan 13, 2022

uarlouski Jan 13, 2022

valfirst Jan 13, 2022

ikalinin1 Jan 13, 2022

ikalinin1 Jan 13, 2022

valfirst Jan 13, 2022

codecov bot commented Jan 13, 2022 •

edited

Loading

uarlouski Jan 14, 2022

valfirst Jan 14, 2022

ikalinin1 Jan 14, 2022

ikalinin1 Jan 14, 2022

uarlouski Jan 14, 2022 •

edited

Loading

uarlouski Jan 14, 2022

ikalinin1 Jan 14, 2022

uarlouski Jan 14, 2022

valfirst Jan 14, 2022

ikalinin1 Jan 14, 2022

valfirst Jan 14, 2022

sonarqubecloud bot commented Jan 14, 2022

	return "application is not started";
	return "session is not started";

Standardize dynamic variables API #2376

Standardize dynamic variables API #2376

Conversation

valfirst commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 13, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uarlouski Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 14, 2022

codecov bot commented Jan 13, 2022 •

edited

Loading

uarlouski Jan 14, 2022 •

edited

Loading