Testing Framework + misc changes #160
Conversation
…eye distance isn't consistent
…so color is our best bet
Add and remove files for PR Replace more files with upstream/master version, update readme, set symlink webgazer.js for webgazerExtractor to use Fix webgazers, edit collision.html to separate js from html
Update commit hash request in readme
Forgot to update the website instructions too.
* Fix webgazer website + commit hook Readd dist directory
|
@Skylion007 Can I get a review on this? |
package-lock.json
Outdated
| @@ -422,6 +418,7 @@ | |||
| "sprintf-js": "~1.0.2" | |||
| } | |||
| }, | |||
| >>>>>>> d3a281f... Refactor and clean | |||
There was a problem hiding this comment.
this file has a number of merge conflict tags in it. could you clean it up a bit? thank you!
There was a problem hiding this comment.
done - interestingly enough it passes the tests with those conflicts.
|
Hi Robin. We're definitely looking into this. We're a bit slower right now since this is a large changelog so there's lots to review, but will be getting to it. Thanks again for your contribution. |
|
Sorry for the delay in getting back, been busy with ECCV. The script doesn't work on OSX, which seems avoidable. It complains about |
|
OK - I changed the netstat to lsof which should work. I also modified the sed command based on this: Unfortunately I don't have the ability to test on OSX myself. |
|
So it almost works, one other issue is that |
| @@ -0,0 +1,24 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
With the proper node utilities, it should be possible to get this part to work even on Windows.
There was a problem hiding this comment.
I have done some testing in windows and attempted to rewrite the testing script as a .bat but I cannot verify it works because #162 - I couldn't get webgazerExtracClient.py to work with windows.
|
What chained in the latest force push? |
|
It should just be the devDependencies package.json (and package-lock and the dist files), currently working on getting the .sh file to run cross platform. EDIT - it was actually just a linefeed change |
|
@RobinReborn Apparently, this is a known issue on Windows with a fix in this thread: if sys.platform == 'win32':
asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) |
|
I did more work on windows compatibility but discovered that there's also an issue with the symlinks we are using - and there was also a husky/npm issue that prevented me from commiting. Given the obstacles I am not sure if it is worth making this compatible with Windows. I did investigate https://www.npmjs.com/package/shelljs but that doesn't support all of the commands I'd like to use. EDIT: additionally there are issues with fsevents (a dependency in package-lock.json) and Windows) |
|
Yeah, there is a fix in progress for that.
…On Mon, Aug 31, 2020 at 9:34 PM Robin Gane-McCalla ***@***.***> wrote:
I did more work on windows compatibility but discovered that there's also
an issue with the symlinks we are using - and there was also a husky/npm
issue that prevented me from commiting.
Given the obstacles I am not sure if it is worth making this compatible
with Windows.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#160 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPVMX7IEC2DCOAUDKMHR3TSDRFQRANCNFSM4QKVQXCQ>
.
|
|
So do you want to make an issue for Windows compatibility? As it is a windows user should be able to run the tests by manually running both servers (and copying dist/webgazer.js to www/data/src/ ) I'm not sure what I should do to finish this PR - I could revert my last few commits and bring it to a state where it works on Linux and MacOS. But the Windows issues have been difficult and it seems like it would probably be easier just to dockerize this project (but that should be another PR). |
|
Just revert it to having UNIX support and we'll create a separate PR for
Windows support.
…On Tue, Sep 1, 2020 at 9:42 PM Robin Gane-McCalla ***@***.***> wrote:
So do you want to make an issue for Windows compatibility?
As it is a windows user should be able to run the tests by manually
running both servers (and copying dist/webgazer.js to www/data/src/ )
I'm not sure what I should do to finish this PR - I could revert my last
few commits and bring it to a state where it works on Linux and MacOS. But
the Windows issues have been difficult and it seems like it would probably
be easier just to dockerize this project (but that should be another PR).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#160 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPVMX64N7FXVKKFPU3UOE3SDWPGNANCNFSM4QKVQXCQ>
.
|
This reverts commit 366e4ec.
I have a basic testing framework that tests the top level webgazer functions and basic regression.
There's a script to automatically run the tests called by npm run test - it won't work on Windows.
I've made a few minor changes outside the scope of testing, there were a few things I noticed while creating the tests that I considered changing (like the structure of index.mjs - right now it has global variables and I think it would be better off if they were all class variables) but I didn't want to change too much until we had agreed on a method of testing so that any incorrect changes made could be detected by tests.
Also for my fix of issue #159 I need the webgazer website ( https://webgazer.cs.brown.edu/data/ ) to include the current readme.md