gasparl / citapp_pc

CITapp: a response time-based Concealed Information Test lie detector web application. This method aims to reveal whether or not a certain information detail (e.g. a photo taken from the scene of a recent burglary) is known to the tested person.
https://gasparl.github.io/citapp_pc/CITapp.html
BSD 2-Clause "Simplified" License
1 stars 3 forks source link

[REVIEW] indepth #9

Closed andytwoods closed 5 years ago

andytwoods commented 5 years ago

Hi, well done writing this. Hard to achieve, this sort of thing. Some comments: -in the readme, the pic links are broken and are not shown. -software engineers view 'automated tests' quite differently from your 'simulated human' tests (although your simulated human tests were very worthwhile developing). The engineer would use software (e.g. travis?) to run a batch of very specific tests to run upon each git push, to ensure things are not broken. These specific tests typically would test their code on a function by function basis (unit testing) and in bigger chunks of functionality (many functions). I personally use Tape when testing JS (there a many testing frameworks though; https://medium.com/javascript-scene/why-i-use-tape-instead-of-mocha-so-should-you-6aa105d8eaf4). I would advise creating some tests for the main functions in your code for peace of mind, and for future proofing.

gasparl commented 5 years ago

Hi, thank you for your review!

Regarding the images in the README, I just forgot to push them; now corrected.

Regarding the testing, I now wrote a function that does the following (quoted from the updated README):

" [...] executes all main functions that initiate the test, prepare the conditions, and, most importantly, generate the test items. The start page input fields are filled automatically using the Demo data and settings. The function also checks whether, in case of either CIT version (standard or enhanced), the expected number of items are generated with the expected item texts (i.e., with the words that are entered on the start page). If no error occurs, the approval message is logged in the Console. Additionally, the function may be given a single argument: the number 1, or the number 2. Entering either will automatically initiate human simulation (with apptest_probe_delay set to 25) after (and if) the previously described test was passed. Choosing dev_test(1) will complete the standard version, while dev_test(2) will complete the enhanced version. In case of no issues, the test should complete itself automatically, all the way until the end of test page. "

I also noted this test as a requirement in the CONTRIBUTING.md.

Now, I also checked out Tape, and I think I could implement this function in it quite straightforwardly - but I wouldn't think this elaboration (with its 2.4 MB extra weight) is necessary for this relatively simple and light application.

In any case, let me know if you are alright with this.

vsoch commented 5 years ago

hey @gasparl ! I'm just peeking in on the review - the tests you added are an improvement, and I think what @andytwoods is looking for is automated testing, namely via continuous integration. This would mean that you wouldn't need to install / include a library like tape here, but you might have it set up with a service like CircleCI or Travis CI so your tests are run with each pull request / merge into master. Here are some links that might be helpful (personally I like CircleCI):

I'm green to many of these testing suites (although I've done testing with selenium and python!) but I'd be happy to help (or try!) if you decide on a strategy that you like best. Minimally, you could create a CI configuration that opens a headless browser, issues the testing functions, and gets back some result.

vsoch commented 5 years ago

It might seem like a hassle now, but I can assure you down the line it makes life so much easier! Because whenever you do a change, or someone opens a PR, all the tests are run automatically (without you needing to think) and you are alerted in the CI (directly in the pull request interface) if a new change has made something go awry.

gasparl commented 5 years ago

The hassle is no problem, I'm just trying to keep things as simple as possible. I opened a question on SO for this: https://stackoverflow.com/questions/54372069/simplest-way-to-test-javascript-code-for-web-frontend-on-each-git-push Either of you is welcome not only to answer/comment but also to edit the question if you wish.

If there is no good answer soon, I will probably just use Travis, that seems the most straightforward based on what I read so far.

vsoch commented 5 years ago

I've heard good things about sauce labs too, they are free for open source! Some docs here and my colleague's repo where he uses it -> https://github.com/FelixHenninger/lab.js/blob/master/.travis.yml.

vsoch commented 5 years ago

CirceCI

vsoch commented 5 years ago

hey @gasparl I'm working on helping to add a continuous integration set up to run the tests, and I've tested the dev_test(1) and dev_test(2) and I think this will work! What we need is some signal from the browser (that the headless tester robot of sorts) can check for to know if the tests are done (and then if they have failed). For example, I would want to be able to issue there commands to the browser:

# start tests, maybe don't return anything
dev_test(1)

# then every 10 seconds, check test status
return tests_complete;

# if the above is True, we finish the script and everything passes / green. If not, we can ask for an additional variable (or run a function) to get the failing result.

completed = self.browser.execute_script("return tests_complete");
if completed:
     success = self.browser.execute_script("return tests_success");
     if not success:
          # and then we would want a function or variable to grab here to print for the user to see.

Does that make sense? This is a bit different than I think a normal testing scenario (that has different modular tests that are run individually) but since you have a cohesive, timed task, I think it would be okay to have it run in one long test as long as we are able to see the result, and if there is a failure, to see why. Also note that I'm using a chrome driver since that seemed to be an important detail.

Let me know what you would like to try!

gasparl commented 5 years ago

@vsoch you are super nice helping out with this. How exactly does this work, how do I initiate the tests?

I don't think the dev_test(1) and dev_test(2) are necessary to be run on every change, I would just put dev_test() in case of git push. The former two is mainly for checking (by eye) that the display changes as expected. I can hardly imagine an automated test for that. Still, if you want I can of course add some value that is returned in case of a successfully completed test.

In general, let me know if I can add anything.

The 15 MB Chrome driver is not too great, but okay I can live with it.

By the way, I've been trying in a different direction, I just arrived at a working draft with Travis and Jasmin/Karma (https://github.com/gasparl/citapp_pc/tree/issue9), but it'd be still long way to go and I'd be happy to abandon it.

vsoch commented 5 years ago

hey @gasparl ! If you have a method you are working on, then please continue! I'm not a professional front end developer so the most I can help do is running a headless browser with selenium, and having Python be the controller.

Basically, think of a headless browser starting up, and we can navigate to the experiment page. We can also feed the dev_test() into the browser, and get things running. The challenge, however, is getting the result of the tests back to the controller. There must be an additional command we can send to the browser (meaning equivalent to something we could type into the javascript console) to get a variable that tells us if tests are complete and passing. Since the testing takes some time, we would need to just check periodically. So this is what I'm asking - I need dev_test() to change some variables that I can check every once in a while to get an update, OR a separate function to run that returns some result that I can parse. Does that make sense? The tests are running headless so there is nothing to look at the browser, or determine that anything is being printed (unless we directly issue a command that can ask for a specific variable).

gasparl commented 5 years ago

@andytwoods, @vsoch: I added the dev_test() (see my first comment) to be automatically run on each push via Travis, using Karma/Jasmin/HeadlessChrome.

No libraries or any external files are included (or needed) in the app.

@vsoch: Nonetheless, if you are still curious whether your solution can also work, I can of course easily create a variable that changes when the test reached the end with success. So, if I understand what you need, I could e.g. assign a window.test_completed = false variable, that would become true in the end.

Also, I think it wouldn't be hard to implement dev_test(1) and dev_test(2) in my current solution, but it doesn't seem very useful. All the main functions are covered with the simple dev_test(), the rest is just small HTML display manipulation, that is tricky to really check via pure coding.

vsoch commented 5 years ago

Great! Using karma will be much better. Could you please walk me through how it works? From your travis.yml https://github.com/gasparl/citapp_pc/blob/master/.travis.yml and the output in the console, it's not clear where the testing is happening (there is no instruction in the travis.yml, and in the output it looks like it just starts and then stops a browser?) The entire testing times are all zero seconds (or very tiny) as well - is the task supposed to take some time?

vsoch commented 5 years ago

Okay here is what I think is going on:

"scripts": {
    "test": "karma start test/karma.conf.js --single-run"
  },
gasparl commented 5 years ago

Yes. The Jasmine test itself is as simple as it can be:

describe("suite", function() {
  it("check if dev_test() returns true", function() {
    expect( dev_test() ).toBe(true);
  });
});

This is in test/test.js.

What "triggers" the test is the dev_test() function here, which returns true if it passed through all main functions (preparing basic variables, CIT conditions, generating all items, initiating first block).

This does not involve any of the time-consuming parts, i.e. where participants have to respond to the items. That part is really quite simple, basically .show() and .hide() the items in the center on keypresses.

So all these tested functions are executed basically instantaneously.

vsoch commented 5 years ago

ah understood - so the functions are triggered as soon as the browser loads I take it? We can open up discussion to add @andytwoods but would it hurt to add dev_test(1) or dev_test(2) since (when I tested the experiment, as much as I can remember) this seemed to actually go through the experiment (and thus would trigger errors if something was broken beyond loading). @andytwoods what are your thoughts?

gasparl commented 5 years ago

ah understood - so the functions are triggered as soon as the browser loads I take it?

Yes.

We can open up discussion to add @andytwoods but would it hurt to add dev_test(1) or dev_test(2) since (when I tested the experiment, as much as I can remember) this seemed to actually go through the experiment (and thus would trigger errors if something was broken beyond loading).

I can give it a try.

gasparl commented 5 years ago

I now added both dev_test(1) and dev_test(2). So first the simple dev_test() runs, then dev_test(1), then dev_test(2). If all three are passed, the test succeeds. The simulated responses are displayed throughout the testing, along with some other info.

vsoch commented 5 years ago

Beautiful! Here is the link on Travis for @andytwoods. @gasparl you've definitely checked off all the boxes for my review, let's get this published soon! :)

If you like badges, note that you can grab the Travis Badge in the settings for the repository (on travis) and put in your README.md in case any future person (including yourself!) wants an easy link.

gasparl commented 5 years ago

Thanks again @vsoch. (I added the Travis Badge ;) )