Yarden needed updates #170

ilseh · 2017-10-18T12:35:14Z

After uppgrading to the latest version, we had some issues for which i made fixes.
We were referencing values that were not accessible so i made them accessible.
Added SftpFileUtil to enable ReadFileFromFtpServerFixture to use sftp instead of ftp.
Added valueOfIsValid to BrowserTest to see if the value in an input field is valid or not (according to the pattern attribute).

Some of our fixtures need these methods.

Added SftpFileUtil so ReadFileFromFtpServerFixture can also handle sftp location (set sftp to true/yes/1).

TimerFixture is often failing locally because the thread wait seems to be off by 1 ms compared to the apache stopwatch. So locally i want to ignore this test but do not want it committed (yet).

# Conflicts: # pom.xml

This reverts commit 736ca68.

…res.git into yarden

fhoeben · 2017-10-18T13:50:16Z

pom.xml

@@ -6,7 +6,7 @@

 	<groupId>nl.hsac</groupId>
 	<artifactId>hsac-fitnesse-fixtures</artifactId>
-	<version>3.6.8-SNAPSHOT</version>
+	<version>3.6.8-Yarden</version>


I'm not going to merge that version number :-)

I will change it to 3.6.8-SNAPSHOT

fhoeben · 2017-10-18T13:51:25Z

pom.xml

@@ -44,6 +44,8 @@
 		<selenium.version>3.6.0</selenium.version>
 		<slf4j.version>1.7.25</slf4j.version>
 		<apache.http.version>4.5.3</apache.http.version>
+        <apache.commons.vsf2>2.0</apache.commons.vsf2>


As these properties are only used once, why not use them in the dependencies?

I often put versions in the properties because when you have a lot of dependencies, it can be nice to see the versions at one glance. However it's more a 'being used to' then really a practice.
I will update.

fhoeben · 2017-10-18T13:52:30Z

src/main/java/nl/hsac/fitnesse/fixture/Environment.java

@@ -1,5 +1,16 @@
 package nl.hsac.fitnesse.fixture;


Why did this class need changing? As far as I can tell you added one method, which you never call. Why is it important to expose the whole symbols map?

At Yarden we normally run the tests on our DEV environment. However during an upgrade of a system, we were requested to run some tests also on the TEST environment. To facilitate this, we created two fixtures; one to determine properties per environment and one to be able to switch to DEV or TEST properties with a single line update.
To be able to to this, we needed to able to reference the symbols in the Environment.

fhoeben · 2017-10-18T14:20:23Z

src/main/java/nl/hsac/fitnesse/fixture/slim/web/BrowserTest.java

+    public boolean valueOfIsValid(String place) {
+        T element = getElementToRetrieveValue(place, null);
+        List<T> invalidElements = findAllByCss("input:invalid");
+        Collection<String> attributesToCompare = Arrays.asList("name", "id");


Does this work for elements that don't have an id or name?

I've added explicit checks on id and name attributes. If not on element, i now throw an exception.

fhoeben · 2017-10-18T14:20:50Z

src/main/java/nl/hsac/fitnesse/fixture/slim/web/BrowserTest.java

+     * @param attributesToCompare list of attribute names of which we compare element with elementList entries.
+     * @return true if theElement was found in elementList (on basis of attributesToCompare)
+     */
+    boolean containsElement(List<T> elementList, T theElement, Collection<String> attributesToCompare) {


please make it protected

fhoeben · 2017-10-18T14:21:08Z

src/main/java/nl/hsac/fitnesse/fixture/slim/web/BrowserTest.java

+     * @param attributesToCompare list of attributes names on which we are going to compare el1 and el2
+     * @return true if el1 is considered to match el2 on basis of attributesToCompare
+     */
+    boolean isMatch(WebElement el1, WebElement el2, Collection<String> attributesToCompare) {


please make it protected

fhoeben · 2017-10-18T14:29:23Z

src/main/java/nl/hsac/fitnesse/fixture/slim/web/BrowserTest.java

+     */
+    public boolean valueOfIsValid(String place) {
+        T element = getElementToRetrieveValue(place, null);
+        List<T> invalidElements = findAllByCss("input:invalid");


Don't you need to check whether element is not null?
Furthermore, would this check also work (and be more efficient) using something like element.findElements(By.cssSelector("input:invalid")).isEmpty() or does the cssSelector only find nested elements and not element itself?

I'll add the null check on the element. I've checked the element.findElements(By.cssSelector("input:invalid")).isEmpty(), but as far i can see, it only looks for child elements.

fhoeben · 2017-10-18T14:29:47Z

src/main/java/nl/hsac/fitnesse/fixture/slim/web/BrowserTest.java

+     * @param place to check
+     * @return true if content of place is valid (according to validation pattern)
+     */
+    public boolean valueOfIsValid(String place) {


Can you add some documentation and acceptance test for this new method?

fhoeben · 2017-10-18T14:33:39Z

src/main/java/nl/hsac/fitnesse/fixture/fit/ReadFileFromFtpServerFixture.java

@@ -15,43 +17,60 @@
    private static final String PASSWORD_KEY = "password";
    private static final String FILE_PATH_KEY = "file";
    private static final String LINES_COUNT_KEY = "nrOfLines";
+    protected static final String IS_SFTP = "sftp";


Can you add usage of this new column to 'HsacExamples.FitTests.ReadFileFromFtpServerFixture'? And maybe also some documentation on what needs to be configured to make sftp work (I expect stuff like adding keys to keystores)?
Is there a reason this one is protected while others are private (only for test?)

I will update the example. This implementation only supports username/password so no adding of keys to keystores.
The value is now protected so it can be used in the test and i don't think it can do harm if subclasses can use this value.

For uniformity I suggest to make all of them protected.

Did requested refactoring and added requested documentation. See #170

The FileFixtureTest was not working on Windows because a linux path seperator was used. When correcting the test, it seemed some handles were not closed (could not remove file) so fixed this in FileFixture. In addition we seem to be able to use org.apache.commons.io.FileUtils.copyFile, instead of copying the contents ourselves.

fhoeben · 2017-10-22T19:47:05Z

src/main/java/nl/hsac/fitnesse/fixture/Environment.java

+    /**
+     * @return the symbols
+     */
+    public ConcurrentMap<String, String> getSymbols() {


Would a return type of Map also suffice?
Since FitNesse is not thread safe I expect to change the concurrent maps to hash maps at some time in the future...

fhoeben · 2017-10-22T19:48:40Z

src/main/java/nl/hsac/fitnesse/fixture/fit/ReadFileFromFtpServerFixture.java

@@ -15,43 +17,60 @@
    private static final String PASSWORD_KEY = "password";
    private static final String FILE_PATH_KEY = "file";
    private static final String LINES_COUNT_KEY = "nrOfLines";
+    protected static final String IS_SFTP = "sftp";


For uniformity I suggest to make all of them protected.

fhoeben · 2017-10-22T20:05:42Z

wiki/FitNesseRoot/HsacExamples/SlimTests/BrowserTests.wiki

@@ -1,8 +1,8 @@
---
+---


This change of the first line (not quite sure what it is) breaks the page on my machine. Can you revert that?

Ok. Don't know what the difference is but will try to revert.

fhoeben · 2017-10-22T20:08:33Z

src/main/java/nl/hsac/fitnesse/fixture/slim/web/BrowserTest.java

+        checkElementHasAttributes(element, attributesToCompare, place);
+
+        List<T> invalidElements = findAllByCss("input:invalid");
+        return !containsElement(invalidElements, element, attributesToCompare);


Why do you need the (complex) logic checking name and id attributes? I ran your acceptance check and it also passes if I just do a simple !invalidElements.contains(element)

No idea why i did that. Will update.

fhoeben · 2017-10-22T20:09:58Z

wiki/FitNesseRoot/HsacExamples/SlimTests/BrowserTests.wiki

@@ -123,7 +123,7 @@ The [[heuristic to find places][>FindingByHeuristics]] tries a number of transla
 |switch to frame <technical selector>                                      |Activates child frame of current (i)frame found using 'technical selector'.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                            |
 |switch to parent frame                                                    |Activates parent frame of current (i)frame. Does nothing if when current frame is the main/top-level one.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              |
 |set implicit find in frames to <true/false>                               |Controls whether !-BrowserTest-! should look for elements inside nested (i)frames if they cannot be found in the current frame (default is 'true').                                                                                                                                                                                                                                                                                                                                                                                                                                                    |
-
+|value of <place> is valid                                                 |Checks if entered value in input field is valid according the pattern attribute. If 'place' doesn't resolve to an element, an exception is thrown. If the field doesn't has both the id and name attributes set, an exception is thrown.                                                                                                                                                                                                                                                                                                                                                               |


Please change description to 'Checks whether entered value in input field ('place') is valid according to its pattern attribute.'

fhoeben · 2017-10-22T20:12:38Z

src/main/java/nl/hsac/fitnesse/fixture/slim/web/BrowserTest.java

+     * @return true if content of place is valid (according to validation pattern)
+     */
+    public boolean valueOfIsValid(String place) {
+        T element = getElementToRetrieveValue(place, null);


Why not use T element = waitUntil(d -> getElementToRetrieveValue(place, null)); to ensure waiting is done before deciding the element could not be found?
Then the if/throw is no longer needed

Nice! I will update.

fhoeben · 2017-10-22T20:13:49Z

wiki/FitNesseRoot/HsacAcceptanceTests/SlimTests/BrowserTest/ValueOfIsValid.wiki

@@ -0,0 +1,59 @@
+This test ensures we can check if an entered value is valid, if a pattern attribute is specified to do the validation.


Please start file with

--- Test ---

To ensure wiki sees it as test page

Not sure what is the difference but hopefully this will fix issue for fhoeben (pages was broken on his machine).

Updated testGetAndSetDirectory. It was successful when FileFixtureText was run on it's own but when run as part of the total junittest, the working dir was different.

On my machine, the waitMilliseconds could return a millisecond before the TimerFixture.timeOnTimer. By letting it also look at System.nanoTime (what the StopWatch in TimerFixture is also using) during the Thread sleep, it is guaranteed the passed time is at least the specified milliseconds.

fhoeben · 2017-10-23T19:57:11Z

pom.xml

@@ -124,6 +124,16 @@
 			<version>3.2.5</version>
 		</dependency>
 		<dependency>
+			<groupId>org.apache.commons</groupId>
+			<artifactId>commons-vfs2</artifactId>
+			<version>2.0</version>


I see there also is a 2.2 version, are you sticking to 2.0 intentionally?

No, when we added the dependency locally, the 2.0 was the latest version.

fhoeben · 2017-10-24T07:01:31Z

src/main/java/nl/hsac/fitnesse/fixture/util/FtpFileUtil.java

-     * @param filePath file to get size for.
-     * @return file's size if found.
-     * @throws RuntimeException in case any exception has been thrown.
+public class FtpFileUtil implements RemoteFileUtil {


One more thought: I believe apache vfs2 also supports ftp. Would it be hard to rewrite FtpFileUtil to also use vfs2, so that the dependency on commons.net could be removed?

I don't know, haven't looked into it. Can it be a todo? I don't know when i will have the time.

I don't have access to an ftp server to test whether things work after refactor, so I was hoping you could do this before moving away and forgetting all about it.....

I had a look. I'm a bit worried about FtpFileUtil.executeCommandOnFTPServer. Do you want to keep this method or can it be removed?

I don't have any tests using it. Does Yarden have any? (I believe the original version of this code also came from you....)

If you don't need it, and it makes migration easy, I'm willing to let them go.

Will you have a chance to look at it it further (to see whether commons.net can be removed)? Or ...?

Unfortunately i didn't have the time anymore to look at it :(

ilse.haanstra and others added 13 commits October 12, 2017 13:08

Changed methods to public.

eff4c23

Some of our fixtures need these methods.

Changed methods to public.

40790a8

Some of our fixtures need these methods.

Added sftp support to ReadFileFromFtpServerFixture

8d98b15

Added SftpFileUtil so ReadFileFromFtpServerFixture can also handle sftp location (set sftp to true/yes/1).

Merge remote-tracking branch 'origin/yarden' into yarden

4752ef8

Undo ignore

6659159

TimerFixture is often failing locally because the thread wait seems to be off by 1 ms compared to the apache stopwatch. So locally i want to ignore this test but do not want it committed (yet).

Correction of error string.

b54527c

Added valueOfIsValid to check if content of a (input) field is valid.

5329b2f

Merge remote-tracking branch 'remotes/upstream/master' into yarden

bb1c978

# Conflicts: # pom.xml

Ignore eclipse files.

63108b0

Ignore eclipse files.

736ca68

Revert "Ignore eclipse files."

8eea85b

This reverts commit 736ca68.

Merge branch 'yarden' of https://github.com/ilseh/hsac-fitnesse-fixtu…

28e7deb

…res.git into yarden

Cleanup of pom before doing pull request.

83a52c9

fhoeben requested changes Oct 18, 2017

View reviewed changes

ilseh added 3 commits October 19, 2017 16:35

Refactoring and documentation

bfb8a00

Did requested refactoring and added requested documentation. See #170

Correction of the test after correction of the FileFixture

b86f962

fhoeben requested changes Oct 22, 2017

View reviewed changes

ilseh and others added 7 commits October 23, 2017 14:13

Character correction.

94e6870

Not sure what is the difference but hopefully this will fix issue for fhoeben (pages was broken on his machine).

Refactoring and junit start path issue fix.

c385b26

Updated testGetAndSetDirectory. It was successful when FileFixtureText was run on it's own but when run as part of the total junittest, the working dir was different.

Little refactoring to make it more consistent.

f11c677

Updates as requested by fhoeben.

d322e8b

Remove documentation which is no longer true

fb6c130

Move documentation for 'value of ... is valid' closer to 'value of ...'

d4e2c27

fhoeben approved these changes Oct 23, 2017

View reviewed changes

fhoeben reviewed Oct 23, 2017

View reviewed changes

Latest commons-vfs2 (2.2)

01f8e40

fhoeben reviewed Oct 24, 2017

View reviewed changes

		@@ -0,0 +1,59 @@
		This test ensures we can check if an entered value is valid, if a pattern attribute is specified to do the validation.

Yarden needed updates #170

Are you sure you want to change the base?

Yarden needed updates #170

Conversation

ilseh commented Oct 18, 2017

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

ilseh Oct 19, 2017 • 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

ilseh Oct 19, 2017 • 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

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

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

fhoeben Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilseh Oct 19, 2017 •

edited

Loading

ilseh Oct 19, 2017 •

edited

Loading

fhoeben Oct 24, 2017 •

edited

Loading