Diagram gui #54

artbez · 2016-11-03T14:39:31Z

To solve #53 issue.
For diagram creating in GUI-tests we need to analyze html code and manipulate it.
Add some instruments to make it more simple.

1. Add spring framework support 2. Add Opener and Auther services 3. Add PropertySourcesPlaceholderConfigurer 4. Add some auth and open page tests

1. Create Scene service 2. Create Pallete service 3. Add dragAndDrop test

1. Add remove method 2. Add move method 3. Test them 4. Add getPosition method 5. Add JavaDoc

1. Add logging to Scene 2. Add logging to Pallete

1. Now scene can link two elements 2. Add test for it 3. Add JavaDoc

1. Add setProperty 2. Add getProperty 3. Add test of them

1. Add SceneWindow class which describes part of the scene that user see 2. Add moveElement test

1. Code refactor 2. Add moveToCell method in Scene 3. Add focus method in Scene 4. Add tests

1. Add test with 3 nodes and 2 links 2. Add Exception for authorization

1. Fix movement bug 2. Fix remove element bug

1. Change names of nodes to string values, which are shown in browser 2. Add moveSomeNodes test

1. Make checkbox property working 2. Add fillProperty test 3. Removed unused package

1. Add Block and Link providers 2. Add Focus and Move helpers

1. Fix bug with movement 2. Add scrolling by movement on the client side 3. Code refactor

jzuken · 2017-03-13T10:10:26Z

editor-core/src/main/webapp/app/core/editorCore/controller/SceneController.ts

+            this.scene.getLinkById(cellView.model.id);
+        var sceneWrapper: HTMLDivElement = <HTMLDivElement> $(".scene-wrapper")[0];
+        var boundingBox: any = sceneWrapper.getBoundingClientRect();
+        if (element instanceof DefaultDiagramNode) {


I guess we better move this code to a new method with a nice and understandable name. or even into several methods

jzuken · 2017-03-13T10:16:13Z

editor-core/src/main/webapp/app/core/editorCore/model/Scroller.ts

+
+class Scroller {
+
+    private _scroll: boolean;


why start using underscore prefix here? we haven't used it before, as far as I know

jzuken · 2017-03-13T10:46:39Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/PageFactory.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class PageFactory {


javadoc comments are welcome :) at least for classes

these comments should describe not only what a class or a method does, but also what other classes use it and how

jzuken · 2017-03-13T10:55:09Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/dia/pallete/Pallete.java

+public interface Pallete {
+
+    /**
+     * Chose element from Pallete.


"Chooses an element"

jzuken · 2017-03-13T12:27:51Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/dia/scene/Coordinate.java

+
+    public static final int POINT_IN_CELL = 25;
+
+    private final int xAbsolute;


I guess final field's name should be in caps

By "5.2.7 Local variable names" of the Style guide we should use lowerCamelCase.

jzuken · 2017-03-13T12:35:14Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/dia/scene/elements/SceneElementImpl.java

+
+public class SceneElementImpl implements SceneElement {
+
+    private By by;


This looks awesome, but I honestly have no clue what it means :)
A comment would be nice.

jzuken · 2017-03-13T12:50:20Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/dia/scene/window/SceneWindow.java

+     * @param element element to move
+     * @param dist position to move
+     * */
+    void move(final Block element, final Coordinate dist) throws ElementNotOnTheSceneException;


would not this method make more sense if it was inside a Block? I mean, block.move() instead of scene.move(block)

jzuken · 2017-03-13T12:51:14Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/dia/scene/window/SceneWindowImpl.java

+
+        Actions actions = new Actions(driver);
+        actions.clickAndHold(element.getInnerSeleniumElement());
+        if (Math.abs(dist.getXAbsolute() - element.getCoordinateOnScene().getXAbsolute()) < sizeHor / 2


these conditions are very hard to understand, we better move them into separate functions

jzuken · 2017-03-13T12:51:46Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/dia/scene/window/SceneWindowImpl.java

+        }
+    }
+
+    private void movement(Actions actions, SceneElement element, OffsetObject offset, Predicate<Coordinate> cond) {


function's name should be a verb

jzuken · 2017-03-13T12:54:01Z

ui-testing/src/test/java/com/qreal/wmp/uitesting/innertests/DiaTest.java

+@RunWith(SpringJUnit4ClassRunner.class)
+@ContextConfiguration(classes = AppInit.class, loader = AnnotationConfigContextLoader.class)
+@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD)
+public class DiaTest {


does 'dia' means 'diagram'?

# Conflicts: # editor-core/src/main/webapp/app/core/editorCore/controller/SceneController.ts # ui-testing/ui-testing.iml

TanVD

Changes requested. One more review needed.

TanVD · 2017-04-14T19:15:25Z

.travis.yml

@@ -68,7 +69,7 @@ script:

  - cd ../ui-testing
  - mvn test -P travis &
-  - sleep 30
+  - sleep 1000


As far as I understand you can just not to use '&' and you will not need sleep

TanVD · 2017-04-14T19:19:30Z

Travis/callTomcat.sh

@@ -0,0 +1,47 @@
+#!/bin/bash


Do we need this file? I did not found any call of this script in changed code.

Yes. This file is needed in case we want to run all services with gui tests.
By this we check that all services are running. You can see that in pom.xml.

TanVD · 2017-04-14T19:23:19Z

Travis/pmd/pmd-ruleset.xml

@@ -11,7 +11,9 @@

    <rule ref="rulesets/java/braces.xml"/>

-    <rule ref="rulesets/java/codesize.xml"/>
+    <rule ref="rulesets/java/codesize.xml">
+        <exclude name="TooManyMethods" />


Why? We do have classes with more than ten business-logic methods?

I break this rule only if I describe services. For example, service Scene provides more than ten methods for users.

TanVD · 2017-04-14T19:27:04Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/Page.java

+
+/** Describes WMP pages in browser. */
+public enum Page {
+    Auth("auth"), Dashboard("dashboard"), EditorRobots("robotsEditor"), EditorBPMN("bpmnEditor");


It's better to have one enum element by line

TanVD · 2017-04-14T19:28:49Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/Page.java

+public enum Page {
+    Auth("auth"), Dashboard("dashboard"), EditorRobots("robotsEditor"), EditorBPMN("bpmnEditor");
+
+    private String identify;


'name' sounds better...

TanVD · 2017-04-14T19:44:35Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/dia/scene/elements/SceneElementImpl.java

+
+import static com.codeborne.selenide.Selenide.$;
+
+/** All Scene elements have selector by which we can clearly define their web instances.


Use standard style of doc if it not fits in one line

TanVD · 2017-04-14T19:46:34Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/dia/scene/providers/BlockProvider.java

+    public void moveToCell(final Block block, final int cellX, final int cellY) {
+        logger.info("Move element {} to cell ({}, {})", block, cellX, cellY);
+        try {
+            sceneWindow.move(block, new Coordinate(cellX * 25, cellY * 25));


Use constant you have already defined (POINT_IN_CELL) instead of magic numbers

TanVD · 2017-04-14T20:34:05Z

ui-testing/src/main/java/com/qreal/wmp/uitesting/services/Auther.java

+/** Used for authentication in current browser session. */
+public interface Auther {
+
+    /** Realizes authentication to the wmp.


I would say 'Implements'

TanVD · 2017-04-14T20:37:29Z

ui-testing/src/test/java/com/qreal/wmp/uitesting/innertests/AuthTest.java

+     * An error must be shown.
+     */
+    @Test
+    public void authWrongTest() {


Random test are bad tests. WrongLogin and WrongPassword variable should be equal for every pass of test.

TanVD · 2017-04-14T20:41:23Z

ui-testing/src/test/java/com/qreal/wmp/uitesting/innertests/AuthTest.java

+     * @return true if it is
+     */
+    private boolean inAuthPage() {
+        return  $(byText("Sign in to continue to Auth")).exists();


And what if I will change this text? You can add your unique flags (in comments, for example) to every page of project. I think it is better solution for identification of page.

Yes, I use urls of pages as an identity flag, but only in next pull request.

Prepare for diagram constructing

9bbb9a6

1. Add spring framework support 2. Add Opener and Auther services 3. Add PropertySourcesPlaceholderConfigurer 4. Add some auth and open page tests

artbez force-pushed the diagram-gui branch from a654f73 to 9bbb9a6 Compare November 3, 2016 14:45

artbez added 2 commits November 3, 2016 19:17

Create Scene and Pallete services

13eb1ef

1. Create Scene service 2. Create Pallete service 3. Add dragAndDrop test

Add exist method to Scene

f88ade9

artbez force-pushed the diagram-gui branch from 38e20c1 to 0f2f45e Compare November 4, 2016 00:03

artbez added 2 commits November 4, 2016 03:19

Add remove and move methods

218c5ff

1. Add remove method 2. Add move method 3. Test them 4. Add getPosition method 5. Add JavaDoc

Add logging

3a55d67

1. Add logging to Scene 2. Add logging to Pallete

artbez force-pushed the diagram-gui branch from 0f2f45e to 3a55d67 Compare November 4, 2016 00:28

Add addLink method

d4e6d8b

1. Now scene can link two elements 2. Add test for it 3. Add JavaDoc

artbez force-pushed the diagram-gui branch from c6e2277 to d4e6d8b Compare November 4, 2016 19:34

Codacy

00ccd70

artbez force-pushed the diagram-gui branch from 8898bfb to 00ccd70 Compare November 4, 2016 20:44

Add PropertyEditor service

69b61d0

1. Add setProperty 2. Add getProperty 3. Add test of them

artbez force-pushed the diagram-gui branch from aba1c1b to 69b61d0 Compare November 4, 2016 22:41

artbez added 2 commits November 14, 2016 20:25

Add possibility to move element to any point in the scene

ec6feba

1. Add SceneWindow class which describes part of the scene that user see 2. Add moveElement test

Code refactor of SceneWindow

a026b52

1. Code refactor 2. Add moveToCell method in Scene 3. Add focus method in Scene 4. Add tests

artbez force-pushed the diagram-gui branch from 3579c4c to a026b52 Compare November 15, 2016 19:35

artbez added 3 commits November 15, 2016 23:32

Merge branch 'master' into diagram-gui

8bc01b9

Create testspace for tasting

3fb78a3

1. Add test with 3 nodes and 2 links 2. Add Exception for authorization

Fix movement bug

d44c095

1. Fix movement bug 2. Fix remove element bug

artbez force-pushed the diagram-gui branch 5 times, most recently from 4a0478c to 678afdc Compare November 23, 2016 11:23

Change namespace of nodes

a55e457

1. Change names of nodes to string values, which are shown in browser 2. Add moveSomeNodes test

artbez force-pushed the diagram-gui branch 2 times, most recently from d8e9f0b to be0e395 Compare November 25, 2016 00:35

Fix bug with property checkbox

60a30c6

1. Make checkbox property working 2. Add fillProperty test 3. Removed unused package

artbez force-pushed the diagram-gui branch from be0e395 to 60a30c6 Compare November 25, 2016 00:57

artbez force-pushed the diagram-gui branch 3 times, most recently from 0eb1692 to 0d8ce72 Compare February 26, 2017 00:39

Add Block and Link providers

447413f

1. Add Block and Link providers 2. Add Focus and Move helpers

artbez force-pushed the diagram-gui branch from 0d8ce72 to 447413f Compare February 26, 2017 01:00

Add additional condition for remove

2441162

artbez force-pushed the diagram-gui branch 7 times, most recently from 4e9cbf9 to ffcae07 Compare February 26, 2017 20:36

Make focus works by JavaScriptExecutor

e77cadb

artbez force-pushed the diagram-gui branch from ffcae07 to e77cadb Compare February 26, 2017 21:20

Add webdriver's timeouts

a001a6d

artbez force-pushed the diagram-gui branch from 80447de to a001a6d Compare February 26, 2017 22:29

Fix bug with movement

e0f2193

1. Fix bug with movement 2. Add scrolling by movement on the client side 3. Code refactor

artbez force-pushed the diagram-gui branch from b03f6ef to e0f2193 Compare March 3, 2017 00:07

Remove unnecessary changes

d2235ef

jzuken requested changes Mar 13, 2017

View reviewed changes

Code refactor, java doc and style guide

fc2fa52

artbez force-pushed the diagram-gui branch from b2250ac to fc2fa52 Compare March 20, 2017 00:38

artbez added 2 commits March 22, 2017 00:44

Merge branch 'master' into diagram-gui

329cbea

# Conflicts: # editor-core/src/main/webapp/app/core/editorCore/controller/SceneController.ts # ui-testing/ui-testing.iml

Merge branch 'master' into diagram-gui

bb65fb9

# Conflicts: # editor-core/src/main/webapp/app/core/editorCore/controller/SceneController.ts # ui-testing/ui-testing.iml

jzuken assigned jzuken and TanVD and unassigned jzuken Apr 10, 2017

TanVD suggested changes Apr 14, 2017

View reviewed changes

Code refactor

2995bc2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diagram gui #54

Diagram gui #54

artbez commented Nov 3, 2016

jzuken Mar 13, 2017

jzuken Mar 13, 2017

jzuken Mar 13, 2017

jzuken Mar 13, 2017

jzuken Mar 13, 2017

jzuken Mar 13, 2017

artbez Mar 19, 2017

jzuken Mar 13, 2017

jzuken Mar 13, 2017

jzuken Mar 13, 2017

jzuken Mar 13, 2017

jzuken Mar 13, 2017

TanVD left a comment

TanVD Apr 14, 2017

TanVD Apr 14, 2017

artbez Apr 27, 2017

TanVD Apr 14, 2017

artbez Apr 27, 2017

TanVD Apr 14, 2017

TanVD Apr 14, 2017

TanVD Apr 14, 2017

TanVD Apr 14, 2017

TanVD Apr 14, 2017

TanVD Apr 14, 2017

TanVD Apr 14, 2017

artbez Apr 27, 2017 •

edited

Loading


		public static final int POINT_IN_CELL = 25;

		private final int xAbsolute;


		public class SceneElementImpl implements SceneElement {

		private By by;


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

		/** All Scene elements have selector by which we can clearly define their web instances.

Diagram gui #54

Are you sure you want to change the base?

Diagram gui #54

Conversation

artbez commented Nov 3, 2016

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

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

TanVD left a 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

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

artbez Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

artbez Apr 27, 2017 •

edited

Loading