-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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.
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
test: module loading error fix solaris #3798 #3855
test: module loading error fix solaris #3798 #3855
Conversation
LGTM |
} | ||
assert.notEqual(e.toString().indexOf(dlerror_msg), -1); | ||
var foundError = false; | ||
dlerror_msg.forEach((errMsgCase) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this whole change be:
assert.strictEqual(dlerror_msg.some((errMsgCase) => {
return e.message.indexOf(errMsgCase) !== -1;
}), true);
There are CI failures for this one |
@cjihrig That looks much simpler, thanks @jasnell But the test itself runs (haven't found a "not ok" on this test on any of the logs unless I'm missing something) duration_ms: 0.183 How would I go about figuring out what's wrong? |
55cc6db
to
1f4aa43
Compare
Can you update the commit message to match the contribution guidelines in https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit. The first line should not exceed 50 characters, the second line should be blank, and the remaining lines should not exceed 72 characters. LGTM other than that. I also started a new CI run since the last one seemed a bit off. https://ci.nodejs.org/job/node-test-pull-request/770/ |
- refactor test to accept multiple error messages per platform - add new message to be found in Solaris 11.3 as per nodejs#3798
1f4aa43
to
4502899
Compare
@evanlucas done. EDIT: seems like there's a lot of PRs having issues with CI at the moment |
Thanks! No, those failures are known issues from flaky tests. |
Landed in 9a628e2. Thanks! |
platform