repl: handle errors from getters during completion

[islandryu]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.07%. Comparing base (b16a0e7) to head (11222b5).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/repl.js	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59044      +/-   ##
==========================================
+ Coverage   90.05%   90.07%   +0.01%     
==========================================
  Files         645      645              
  Lines      189153   189162       +9     
  Branches    37091    37099       +8     
==========================================
+ Hits       170348   170380      +32     
+ Misses      11528    11478      -50     
- Partials     7277     7304      +27     

Files with missing lines	Coverage Δ
lib/repl.js	94.24% <91.66%> (+0.02%)	⬆️

... and 45 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[islandryu]

I noticed this while trying to add a test for a getter in the following PR, it seems that if an error is thrown inside a getter, the server itself crashes.
#58943

@dario-piotrowicz
I'd appreciate it if you could take a look.

[dario-piotrowicz]

@islandryu thanks so much for spotting the issue 🙏

The change looks good to me 😃

But I think the test can be cleaned up a bit (it looks more complex than it needs to me in my opinion), in it I see:

• the unnecessary definition of a class
• unnecessary multiple evals
• missing spacing
• duplicated code

I would suggest to change the test to something like the following instead:

'use strict';

const common = require('../common');
const repl = require('repl');
const ArrayStream = require('../common/arraystream');
const assert = require('assert');

(async function() {
 await runTest();
})().then(common.mustCall());

async function runTest() {
 const input = new ArrayStream();
 const output = new ArrayStream();

 const replServer = repl.start({
   prompt: '',
   input,
   output: output,
   allowBlockingCompletions: true,
   terminal: true
 });

 replServer._domain.on('error', (e) => {
   assert.fail(`Error in REPL domain: ${e}`);
 });

 await new Promise((resolve, reject) => {
   replServer.eval(`
     const getNameText = () => "name";
     const foo = { get name() { throw new Error(); } };
     `, replServer.context, '', (err) => {
     if (err) {
       reject(err);
     } else {
       resolve();
     }
   });
 });

 ['foo.name.', 'foo["name"].', 'foo[getNameText()].'].forEach(test => {
   replServer.complete(
     test,
     common.mustCall((error, data) => {
       assert.strictEqual(error, null);
       assert.strictEqual(data.length, 2);
       assert.strictEqual(data[1], test);
     })
   );
 })
}

But besides the test the change looks great to me 😄

[islandryu]

Thanks for the feedback!
Indeed, my test was unnecessarily complex.
I've updated it following your suggestion!

[dario-piotrowicz]

Thanks a lot for updating the test @islandryu 🫶

Looks good to me, again thanks a lot for spotting this! 🚀

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67948/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67973/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/67978/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/68000/