Added functional and unit tests #37

maerteijn · 2018-08-03T11:29:55Z

No description provided.

And improved elapsedToString according to the tests fsdfsfs

And increase the timeout as the first time it can take a while

maerteijn · 2018-08-03T11:31:47Z

@erwindon could you review this pull request before merging? And I have one request for you, see:

https://github.com/maerteijn/SaltGUI/blob/nightmarejs/tests/unit/parsecmdline.js#L21

Could you add some more assertions there? :)

oliverdunk · 2018-08-04T02:53:14Z

This is really cool!

erwindon · 2018-08-04T09:38:54Z

Debian 9:

$ bash runtests.sh      
ERROR: There are no scenarios; must have at least one.
Removes stopped service containers.

By default, anonymous volumes attached to containers will not be removed. You
can override this with `-v`. To list all volumes, use `docker volume ls`.

Any data which is not in a volume will be lost.

Usage: rm [options] [SERVICE...]

Options:
    -f, --force   Don't ask to confirm removal
    -v            Remove any anonymous volumes attached to containers
    -a, --all     Obsolete. Also remove one-off containers created by
                  docker-compose run

any suggestions?

edit: actually only the first line is the actual error. everything else is from the cleanup procedure in the script
edit: the secondary error is from "docker-compose rm" which does not support -s

maerteijn · 2018-08-04T10:47:56Z

@erwindon You have the wrong yarn I think. See: yarnpkg/yarn#2821 and then the second answer, and the installation manual of yarn: https://yarnpkg.com/lang/en/docs/install/#debian-stable

erwindon · 2018-08-04T11:20:01Z

I needed an additional ln -s /usr/bin/nodejs /usr/bin/node
(found on https://askubuntu.com/questions/404929/node-command-not-found)

erwindon · 2018-08-04T11:32:07Z

next one...

$ mocha --bail --trace-warnings --reporter spec tests/unit/ tests/functional/
/usr/bin/nodejs: bad option: --trace-warnings
error Command failed with exit code 9.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I removed --trace-warnings from package.json and now the first test result comes in.

/home/erwindon/SaltGUI/saltgui/static/scripts/parsecmdline.js:109
      let str = "";
      ^^^
SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode

Since this is a complaint about the file we are testing, I guess it is working.
(well, ok that was only the output of yarn lint)

maerteijn · 2018-08-04T12:08:01Z

This sounds like an ancient version of nodejs. Which version do you have installed? See https://nodejs.org/en/download/package-manager/#debian-and-ubuntu-based-linux-distributions how to install version 10. See also the note about nodejs-legacy package

erwindon · 2018-08-04T12:26:08Z

$ nodejs --version
v4.8.2

Seems I cannot trust the Debian distributions any more.
Now upgraded to node.js 8.11.3 and yarn 2.7.6
Much better now!

erwindon · 2018-08-04T12:33:56Z

Still, I need to use:
version 2 in docker-compose.xml
rm -f instead of rm -f -s in package.json
And I'm ignoring a timeout failure in the login test.

maerteijn · 2018-08-04T20:52:42Z

The 3333 was intentional yes as it’s referred to it further in the readme.

maerteijn · 2018-08-04T20:56:47Z

I dropped zombie.js as it took way too much time to write working tests. It is a bit buggy and there is no up to date documentation.

Nightmare.js works pretty awesome and is well documented. Tests can be debugged with a debugger which is pretty neat. The only thing is yes, you’ll need a display driver.

maerteijn · 2018-08-04T20:57:45Z

I would put the updated parser and the tests for it here in this branch

oliverdunk · 2018-08-04T21:18:14Z

tests/functional/login.js

+const url = 'http://localhost:3333/';
+
+
+


Is there a reason for the extra new lines here?

No, I changed it.

oliverdunk

I've left some comments with some feedback, mainly style based rather than around the implementation. It's great to see the work going on with SaltGUI and while I am still too busy to contribute, I want to try and look at PRs more often when I can 😅

Thanks for keeping the project alive! ❤️

oliverdunk · 2018-08-04T21:19:26Z

tests/functional/login.js

+      typeInterval: 20,
+      // uncomment this to show the browser and the debug window
+      // openDevTools: {
+      //   mode: "detach"


It would be great to have the script take in a parameter which could be used to change this setting. The default should probably be a hidden browser window and it should be shown only if requested.

Well, then the bash script should parse them, add them to the testing script which is implicitly invoked with yarn, then I need to parse the command again in javascript.

So I think WAY to much work (and hassle for me) for a small feature in the testrunner. But if you want it you can make a PR of course 😄

oliverdunk · 2018-08-04T21:21:29Z

runtests.sh

+# run the unittests/nightmare.js functional tests
+yarn test
+
+set +e


Missing newline at the end of this file 😉

It's bash, not javascript 😳 but I've added the newline.

oliverdunk · 2018-08-04T21:24:27Z

saltgui/static/scripts/utils.js

-  }
+    if(secondsPassed < 60 * 60 * 24) {
+      var hours = Math.round(secondsPassed / 60 / 60);
+      return hours + " hours ago";


Could we end up with "one hours" here?

oliverdunk · 2018-08-04T21:25:14Z

saltgui/static/scripts/utils.js

+    return "A long time ago, in a galaxy far, far away";
+  }
+  catch(err) {
+    return "It did happen, when I don't know";


We should probably log this error the console?

Yes, I added it.

oliverdunk · 2018-08-04T21:27:48Z

tests/functional/login.js

+
+    it('we should be redirected to the login page', done => {
+      browser
+        .end()


Can you explain the purpose of the end here? I'm not sure I get it.

See https://github.com/segmentio/nightmare#end and the api documented there. See also the beforeEachhandler where we queue the url.

oliverdunk · 2018-08-04T21:31:40Z

tests/functional/login.js

+        .type('#username', 'salt')
+        .type('#password', 'salt')
+        .click('#login-submit')
+        .wait( ()=> {


Missing space after the ()?

Yes I added it.

oliverdunk · 2018-08-04T21:32:55Z

tests/helpers/wait-for-docker.js

+  followRedirect: true,
+};
+
+waitOn(resources, error => {


Since we only use waitOn once, I wonder if we can write our own method to check every few seconds instead? It just seems like there's the opportunity here to avoid a dependency.

Well, the only way to do this with node.js (without adding dependencies) is with the shipped http library. (as fetch does not exist). And this module is really low level, so we have to make all the error handling ourselves (even getting the chunks of partial result data is something you have to do yourselves).

So know I rewrote it with the requests which is already a dependency of one of the other packages, which means we can drop wait-on. (And I did).

oliverdunk · 2018-08-04T21:34:17Z

tests/unit/utils.js

+require('../../saltgui/static/scripts/utils');
+
+
+describe('Unittests for utils.js', function() {


Some tests for potential mistakes would be great here (one minutes etc.)

Yeah well, it would be fantastic but this function is now tested enough I think. PR's are welcome 😃

oliverdunk · 2018-08-04T21:34:55Z

tests/unit/utils.js

+   global.window = new Object({});
+
+require('../../saltgui/static/scripts/utils');
+


Is there a reason for two lines over one here?

No, so I adjusted it.

oliverdunk · 2018-08-04T21:36:04Z

tests/unit/parsecmdline.js

+    assert.equal(args.length, 1);
+    assert.equal(args[0], "NOne");
+    assert.equal(Object.keys(params).length, 0);
+


I know I've left similar comments above, but is there a reason for the newlines after comments, or the multiple lines between code blocks? I just think there is room for some style improvements 😄

The unit tests in tests/unit/parsecmdline.js are grouped in sets. These comments are basically the group titles. 3 empty lines before that to visually separate from the previous group.

I agree with oliver that there were 's a bit too much newlines So I adjusted it a bit.

oliverdunk · 2018-08-04T21:42:40Z

I'm running into an error when trying to use these tests locally:

ERROR: error pulling image configuration: Get https://production.cloudflare.docker.com/registry-v2/docker/registry/v2/...: Service Unavailable

The Cloudflare URL was quite long, so I've intentionally truncated it.

maerteijn · 2018-08-05T06:25:57Z

@oliverdunk Seems like https:///production.cloudflare.docker.com was offline, as travis was able to download them. Could you try again?

I'll review your other comments later. The newlines come from the python style guide, which is unconsciously applied to javascript :-) Would be something to add to the linter btw.

maerteijn · 2018-08-05T14:39:36Z

And now I have to fix the build first :)

maerteijn · 2018-08-05T14:47:41Z

Build is fixed, @oliverdunk can you review again with all the changes? (Please note I already created a new branch which needs to be reviewed later for more explicit linting: nightmarejs...replace-jshint-with-eslint)

maerteijn · 2018-08-09T15:24:52Z

@oliverdunk Any chance to respond the following days? Would be nice to have your feedback and this pull request merged as @erwindon has a lot of work waiting for us 😄

oliverdunk · 2018-08-09T21:40:56Z

Hey @maerteijn, absolutely. Will aim to do so tomorrow. If I'm blocking you and you're happy, feel free to merge without me 👍

Martijn Jacobs added 20 commits August 3, 2018 12:51

Use prebuild docker images, update ubuntu, added README

5c72f0e

Added nightmare.js, chai, mocha and wait-on for testing

77b1d2a

Added helperscript to wait for the dockersetup to be ready

a58cba5

Added id to submit button so we can select it in js

392ff57

Added a unittest for elapsedToString

1f8a4f3

forEach expects an array (at least in Node.js)

25a735b

_callMethod already returns a promise

3073826

Added first tests with nightmarejs

1acf4cf

Update package.json with more testing scripts

152993e

Added a testscript to run all tests

7cb2898

added docker environment to travis and call runtests.sh

22df9b0

Updated README and added testing documentation

0809c69

Updated yarn.lock

f46bc16

Added more unittests and fix global window hack

4ce400e

And improved elapsedToString according to the tests fsdfsfs

Bail on first error

d40f64c

Updated readme with testing frameworks

569bf40

Added a test with valid login credentials

d53fb1c

And increase the timeout as the first time it can take a while

Make sure we always cleanup the docker images

0decd18

Added a virtual display for running the nightmarejs tests

8663c07

Update test description

c355979

maerteijn assigned erwindon Aug 3, 2018

Erwin Dondorp added 2 commits August 4, 2018 23:03

json does not support single-quoted strings

a24bc29

added regressiontestset for parseCommandLine

972fca3

oliverdunk reviewed Aug 4, 2018

View reviewed changes

erwindon assigned maerteijn and unassigned erwindon Aug 4, 2018

oliverdunk requested changes Aug 4, 2018

View reviewed changes

Martijn Jacobs added 8 commits August 5, 2018 16:02

Log error to console, make sure english grammar is correct

073ea03

Dropped wait-for as the script is now rewritten using requests

daf6a3e

Added correct versions in README.md

1f76faf

Fixed some linting suggestions

83a817f

Update test according to new output

e0cb66b

Updated lockfile

3e17741

Don't bail for now

8a2803c

Added newline

8826405

Increase timeout for travis

8e91a90

Martijn Jacobs added 3 commits August 6, 2018 19:20

There are multiple timeout settings in nightmarejs

835c72b

Added a logout test

6881665

Added logout test, fixed some linting

4eb6d42

Use a environment variable to show the browser/debug window

2bb18db

maerteijn merged commit f81c06f into master Aug 19, 2018

maerteijn deleted the nightmarejs branch August 19, 2018 17:15

		require('../../saltgui/static/scripts/utils');


		describe('Unittests for utils.js', function() {

		global.window = new Object({});

		require('../../saltgui/static/scripts/utils');

Added functional and unit tests #37

Added functional and unit tests #37

Conversation

maerteijn commented Aug 3, 2018

maerteijn commented Aug 3, 2018

oliverdunk commented Aug 4, 2018

erwindon commented Aug 4, 2018 • edited

maerteijn commented Aug 4, 2018 • edited

erwindon commented Aug 4, 2018

erwindon commented Aug 4, 2018 • edited

maerteijn commented Aug 4, 2018 • edited

erwindon commented Aug 4, 2018

erwindon commented Aug 4, 2018

maerteijn commented Aug 4, 2018

maerteijn commented Aug 4, 2018

maerteijn commented Aug 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverdunk 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

maerteijn Aug 5, 2018 • edited

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

maerteijn Aug 5, 2018 • edited

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

maerteijn Aug 5, 2018 • edited

Choose a reason for hiding this comment

oliverdunk commented Aug 4, 2018

maerteijn commented Aug 5, 2018

maerteijn commented Aug 5, 2018

maerteijn commented Aug 5, 2018 • edited

maerteijn commented Aug 9, 2018

oliverdunk commented Aug 9, 2018

erwindon commented Aug 4, 2018 •

edited

maerteijn commented Aug 4, 2018 •

edited

erwindon commented Aug 4, 2018 •

edited

maerteijn commented Aug 4, 2018 •

edited

maerteijn Aug 5, 2018 •

edited

maerteijn Aug 5, 2018 •

edited

maerteijn Aug 5, 2018 •

edited

maerteijn commented Aug 5, 2018 •

edited