RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/humble/Concepts/Basic/About-Client-Libraries.html?highlight=rclnodejs#community-maintained
Apache License 2.0
316 stars 70 forks source link

Clean up some tests #725

Closed felixdivo closed 3 years ago

felixdivo commented 3 years ago

This:

Closes #731

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 91.515% when pulling c872494a102d0564fc5cef95957a1951a13e80b1 on felixdivo:rewrite-tests into b09a171b9a97756a79ea93bcc383dfcf2fbe3c5c on RobotWebTools:develop.

felixdivo commented 3 years ago

Hm, strange. I think this is done, but there's this error: FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal. on CircleCI and AppVeyor. Has it appeared before? And Travis is failing too, but seemingly unrelated.

wayneparrott commented 3 years ago

@felixdivo I took a quick look and think the problem might be at line 55 in test-logging.js. image

My reasoning is that the code where the exception is occurs is:

// preprocess Context
  RclHandle* context_handle = RclHandle::Unwrap<RclHandle>(
      Nan::To<v8::Object>(info[0]).ToLocalChecked());
minggangw commented 3 years ago

Hi @felixdivo would you please create the corresponding issues for this PR and #726, so we could track through issue<->pull request, thanks!

minggangw commented 3 years ago

My reasoning is that the code where the exception is occurs is:

// preprocess Context
  RclHandle* context_handle = RclHandle::Unwrap<RclHandle>(
      Nan::To<v8::Object>(info[0]).ToLocalChecked());

That's right, we have to pass an instance of Context as the 1st parameter.

felixdivo commented 3 years ago

Thanks @wayneparrott! It was exactly there. I was fooled by the default value for the context parameter, which (somewhat obviously) isn't used when passing an argument. It's now fixed. I'll check if the rest now works.

felixdivo commented 3 years ago

would you please create the corresponding issues for this PR and #726

Done.

And this PR is ready for a review.