Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NodeJs Binding Tests #3877

Merged
merged 1 commit into from
Apr 4, 2017
Merged

NodeJs Binding Tests #3877

merged 1 commit into from
Apr 4, 2017

Conversation

daniel-j-h
Copy link
Member

@daniel-j-h daniel-j-h commented Mar 29, 2017 •
edited
Loading

For #3851. NodeJS tests.

Tasks

  • Use Monaco
  • Run on Travis
  • Adapt all tests
  • Figure out why build fails to dlopen node bindings on Travis; works locally

@daniel-j-h
Copy link
Member Author

Builds on Travis are failing because the node js build script at script/travis/build.sh is setting its own vars for cmake, e.g. not passing the lto flag.

This will change anyway with #3834 so let's merge this first and rebase then.

@daniel-j-h daniel-j-h force-pushed the nodejs-tests branch 2 times, most recently from e8c5d98 to 3adf1c5 Compare March 30, 2017 13:38
@daniel-j-h
Copy link
Member Author

All adapted, works locally. Waiting for #3834 to land, then needs to be rebased against the changes there.

@daniel-j-h daniel-j-h requested a review from TheMarex March 30, 2017 14:16
TheMarex
TheMarex requested changes Mar 30, 2017
View reviewed changes
Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor stuff about naming could be improved and a little bit more constants refactoring. 👍

.travis.yml Outdated
if [ -n "${ENABLE_NODE_BINDINGS}" ]; then
nvm install 4
nvm use 4
source ./scripts/travis/build.sh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to go, will be removed in #3834

package.json Outdated
@@ -30,10 +29,10 @@
"scripts": {
"lint": "eslint -c ./.eslintrc features/step_definitions/ features/support/",
"test": "npm run lint && node ./node_modules/cucumber/bin/cucumber.js features/ -p verify && node ./node_modules/cucumber/bin/cucumber.js features/ -p mld",
"nodejs-tests": "make -C test/data && ./lib/binding/osrm-datastore test/data/ch/monaco.osrm && node test/nodejs/index.js | faucet",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does faucet do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formats the machine readable tap format output tape generates for humans, much easier to locate failing tests and see why they are failing. Not sure if faucet is the right pkg for this, I just found it and used it. We can change this.

test/nodejs/match.js Outdated
@@ -1,12 +1,14 @@
var OSRM = require('../../');
var test = require('tape');
var berlin_path = require('./osrm-data-path').data_path;
var data_path = require('./osrm-data-path').data_path;
var kLocations = require('./locations').locations;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of kLocations and kRoute I would suggest using more explicit names like:

  • two_test_coordinates
  • three_test_coordinates

test/nodejs/nearest.js Outdated
var berlin_path = require('./osrm-data-path').data_path;
var data_path = require('./osrm-data-path').data_path;
var kLocations = require('./locations').locations;
var kRoute = require('./locations').route;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

test/nodejs/route.js Outdated
var berlin_path = require('./osrm-data-path').data_path;
var data_path = require('./osrm-data-path').data_path;
var kLocations = require('./locations').locations;
var kRoute = require('./locations').route;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

test/nodejs/trip.js Outdated
var berlin_path = require('./osrm-data-path').data_path;
var data_path = require('./osrm-data-path').data_path;
var kLocations = require('./locations').locations;
var kRoute = require('./locations').route;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

test/nodejs/tile.js Outdated
var osrm = new OSRM(berlin_path);
osrm.tile([17603, 10747, 15], function(err, result) {
var osrm = new OSRM(data_path);
osrm.tile([17059, 11948, 15], function(err, result) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tile coordinates should also be refactored to be in a constants module.

test/nodejs/locations.js Outdated
@@ -0,0 +1,8 @@
// Somewhere in Monaco ..
// http://www.openstreetmap.org/#map=18/43.73185/7.41772
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better name would be constants to keep it more general. Might be worth merging this with osrm-data-path.

@daniel-j-h daniel-j-h force-pushed the nodejs-tests branch 3 times, most recently from 675a4ef to 66cf051 Compare March 31, 2017 11:58
@daniel-j-h
Copy link
Member Author

@TheMarex adapted the code to your requested changes. Rebases onto the changes from master.

What I can still see are module loading issues:

Sanitized (asan) libosrm makes some troubles, we may have to ld preload libasan (?)
https://travis-ci.org/Project-OSRM/osrm-backend/jobs/217144024#L1691

undef. sym. typeinfo for std::__1::__shared_weak_count no idea yet where this is coming from
https://travis-ci.org/Project-OSRM/osrm-backend/jobs/217144025#L1690

@daniel-j-h daniel-j-h force-pushed the nodejs-tests branch 6 times, most recently from 46e5e6c to eaaf5e7 Compare April 3, 2017 13:45
@daniel-j-h
Copy link
Member Author

daniel-j-h commented Apr 3, 2017 •
edited
Loading

Asan symbol issue fixed by ld-preload'ing libasan for node eaaf5e7. It detects v8 internal issues now. Disabling it and seeing how Travis likes it.

Symbol issue fixed - we were accidentally compiling and linking the node addon against libc++, because node-cmake in its version 1 we're using sets libc++ as default when the compiler is clang d0ca50a.

TheMarex
TheMarex approved these changes Apr 3, 2017
View reviewed changes
Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor search and replace stuff left. Otherwise looks great. 👍 Finally tests again.

test/nodejs/match.js Outdated
var options = {
coordinates: [[13.393252,52.542648],[13.39478,52.543079],[13.397389,52.542107]],
coordinates: three_test_coordinates.slice(0, 3),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .slice(0,3) doesn't do anything here.

test/nodejs/match.js Outdated
var options = {
coordinates: [[13.393252,52.542648],[13.39478,52.543079],[13.397389,52.542107]]
coordinates: three_test_coordinates.slice(0, 3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

test/nodejs/match.js Outdated
var options = {
coordinates: [[13.393252,52.542648],[13.39478,52.543079],[13.397389,52.542107]],
coordinates: three_test_coordinates.slice(0, 3),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

test/nodejs/match.js Outdated
var options = {
coordinates: [[13.393252,52.542648],[13.39478,52.543079],[13.397389,52.542107]]
coordinates: three_test_coordinates.slice(0, 3),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

test/nodejs/match.js Outdated
var options = {
coordinates: [[13.393252,52.542648],[13.39478,52.543079],[13.397389,52.542107]],
coordinates: three_test_coordinates.slice(0, 3),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

test/nodejs/trip.js Outdated
var options = {
coordinates: [[13.43864,52.51993],[13.415852,52.513191]],
coordinates: [three_test_coordinates[0], three_test_coordinates[1]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two_test_coordinates can be used here.

test/nodejs/trip.js Outdated
var options = {
coordinates: [[13.43864,52.51993],[13.415852,52.513191]],
coordinates: [three_test_coordinates[0], three_test_coordinates[1]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two_test_coordinates can be used here.

test/nodejs/trip.js Outdated
var options = {
coordinates: [[13.43864,52.51993],[13.415852,52.513191]],
coordinates: [three_test_coordinates[0], three_test_coordinates[1]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two_test_coordinates can be used here.

test/nodejs/trip.js Outdated

// fixed start, non-roundtrip
var options = {
coordinates: [[13.43864,52.51993],[13.415852,52.513191]],
coordinates: [three_test_coordinates[0], three_test_coordinates[1]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two_test_coordinates can be used here.

test/nodejs/trip.js Outdated
var options = {
coordinates: [[13.36761474609375,52.51663871100423],[13.374481201171875,52.506191342034576]],
coordinates: [three_test_coordinates[0], three_test_coordinates[1]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two_test_coordinates can be used here.

@daniel-j-h
Copy link
Member Author

daniel-j-h commented Apr 4, 2017 •
edited
Loading

Clang from mason plus ld-preload-ing libasan from the system fails beautifully:

==8136==Your application is linked against incompatible ASan runtimes.

export LD_PRELOAD='/usr/lib/x86_64-linux-gnu/libasan.so.2'

I think we need to package up and use the clang asan support lib?

Not doing asan builds for the node tests for now.

@daniel-j-h
NodeJS Binding Tests
3b00c22 
Does not run the nodejs tests in sanitized builds. We'd have to

    export LD_PRELOAD='/usr/lib/x86_64-linux-gnu/libasan.so.2'

the asan lib. But it seems like our Clang from mason does not like the
system's libasan. Also we'd need a suppression file for v8 and node.
@daniel-j-h
Copy link
Member Author

Ready for 🚀

https://travis-ci.org/Project-OSRM/osrm-backend/jobs/218382022#L1657

@TheMarex TheMarex merged commit 70b3962 into master Apr 4, 2017
@TheMarex TheMarex deleted the nodejs-tests branch April 4, 2017 16:57
@TheMarex TheMarex mentioned this pull request Apr 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheMarex TheMarex TheMarex approved these changes

Assignees
No one assigned
Labels
NodeJs Ready To Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@daniel-j-h @TheMarex