big refactoring

[daymos]

separating the logic from interface has resolved the issue we were having with tests. Now there is no delay on executing the test set. The failing tests are due to the refactoring itself and can be solved without issue ( I think..)

[g-sam]

great work. am about to go out so haven't been able to spend much time on this, but in a brief attempt I couldn't get the tests to show up. might just be a quirk and will hopefully be able to test it properly later this evening.

To note: I can't see you updating the 'lap' div anywhere (which presumably you would be shifting to draws.js). Might be worth trying to do that as that was the line that apparently messed up the tests.

Also looks like the final test now needs to be updated or removed.

[daymos]

yes to be able to run the test you have to clone the 'new' branch of the repo and run npm install, some paths have changed. good point with the lap thing. I have eliminated it for now to try to isolate the issue. Now it is clear that jasmine can indeed handle this many divs. Tomorrow i'll work on the tests, they need radical change and possibly I can try some different tecniques

[g-sam]

yep, tests do appear after installing

[daymos]

if you try this last commit with lap functin it now works!

[g-sam]

coool. it definitely works. the error is still thrown, though – I want to know why!! feel free to merge this whenever you want to.

[daymos]

the issue happens if the unfill function doesnt have time to pop all path svg. My guess is that, given that the dom is like a tree, if some branches are not removed, at the following iteration some unespected changes happend to the dom. and when you call getElementById, it tries to traverse the the tree but cannot find the laps div. Its not a real explanation but to verify it we would need to find a tool to visualize the dom after execution of stop(). I thing we would see some isolated branches that can no longer be reached.

[daymos]

by the way the fix was to increase then number of iteration of unfill to make sure there are no longer these 'dead' dom elements

[g-sam]

If your explanation is right then maybe you could get around it by using a different selector --e.g. nth child, or something that wouldnt necessitate looking down every branch if that is indeed what it does. I'm not sure though -- if rectangle are getting left behind I'd expect to see them on the page. Also, the explanation has to involve jasmine as the errors are only thrown on the test page.

[daymos]

i hadn't seen the error thrown in the console, forget my explanation, I haven't solved the issue!

[daymos]

ok, I got it. If you call document.GetElementById('laps') from the test page, document refers to the jasmine test page, so it returns null.

[daymos]

I guess now it works just because there are less error thrown to clog up the execution of jasmine. In fact before the error was a timeout error

[g-sam]

yes, that's it, well done!! I put a laps div in the test html just to check, and it didn't throw the errors. …which means even after refactoring, logic and display are not properly decoupled. so the correct way to build would be to comment out all calls to sketcher? seems a bit odd.

[daymos]

I think it is decoupled, at least as much as you can. If you run a script in a page different from what it was intended for, you have to change all the 'addresses' of I/O. In an analogous way if you take our js logic file and copy paste it into a different stop watch project it will work,but you will still have to change all the folder paths. I think the issue here is inside of the tests, and one possible solution is to use mocking. Which is just what you did by putting a div in the test html. Another way would be to use spies, so we only test if the system calls the function that contains the line document.getElementById.

[daymos]

I think the concept is that being decoupled is different from being compatible. The logic here is decoupled from interface, but it is only compatible with interfaces that have a div called lap.