Fix hover tooltip failure when a memember starts with a money sign

[Nathaniel-King-Navarrete]

Changes

#3249
I didn't do the the suggested fix, because Tools.qualifyPath() wasn't being exclusively used by PASE commands, and would cause those commands to fail.

How to test this PR

Before:

After:

Checklist

• have tested my change
• have created one or more test cases
• updated relevant documentation
• Remove any/all console.logs I added
• have added myself to the contributors' list in CONTRIBUTING.md

[buzzia2001]

Thanks for the fix! I tested it on a real system (CCSID 280). Names that start with $ look fixed, but a few cases still fail:

• $ in the middle of a name: A$BC generates .../A$BC.MBR, and the shell expands $BC. Library and file names like LIB$X are affected too.
• # names containing $: sanitizeObjNamesForPase generates "#$ABC".MBR, and $ still expands inside double quotes. I confirmed this one on my system.
• The escape runs before sysNameInAmerican: on CCSID 285, the local name £ABC becomes $ABC after inAmerican(), so it reaches the shell unescaped.
• objectResolve has the same bug: /QSYS.LIB/LIB.LIB/$OBJ.*.

A shared helper that escapes every $ (name.replace(/$/g, '\$')), applied after sysNameInAmerican in both methods, would cover all of these.

Could you also add tests?

[Nathaniel-King-Navarrete]

@buzzia2001 I am new to writing test cases. Any feedback for improvements is apperciated. Thanks!

[buzzia2001]

Hi @Nathaniel-King-Navarrete, thanks for adding tests, and for a first go they're in good shape: each one creates its own object, checks the result, and cleans up in a finally. A few suggestions:

1. Collapse the duplicated tests with it.each

The three memberResolve tests (and the three objectResolve ones) are identical apart from the name prefix. Vitest can run the same body over a list of values, so a new case becomes one extra entry in the array:

it.each(['$', '#', '#$'])('memberResolve with %s', async (prefix) => {
  const content = connection.getContent();
  const tempLib = connection.getConfig().tempLibrary;
  const tempSPF = `${prefix}ABCD`.concat(connection.variantChars.local);
  const tempMbr = tempSPF;

  try {
    const result = await connection.runCommand({
      command: `QSYS/CRTSRCPF ${tempLib}/${tempSPF} MBR(${tempMbr})`,
      environment: 'ile'
    });
    expect(result.code).toBe(0);

    const member = await content.memberResolve(tempMbr, [
      { library: 'QSYSINC', name: 'MIH' }, // Doesn't exist here
      { library: 'NOEXIST', name: 'SUP' }, // Doesn't exist here
      { library: tempLib, name: tempSPF } // Does exist here
    ]);

    expect(member).toEqual({
      asp: undefined,
      library: tempLib,
      file: tempSPF,
      name: tempMbr,
      extension: 'MBR',
      basename: `${tempMbr}.MBR`
    });
  }
  finally {
    await connection.runCommand({
      command: `QSYS/DLTF ${tempLib}/${tempSPF}`,
      environment: 'ile'
    });
  }
});

2. Assert that the setup worked

const result = await connection.runCommand(...) is assigned but never checked. If CRTSRCPF / CRTDTAARA fails (e.g. the object is left over from a previous run), the test fails later with a confusing "expected undefined to equal {...}". Adding expect(result.code).toBe(0) right after the setup (as in the example above) makes the real cause obvious.

3. Add a plain unit test for Tools.escapeForShell

Now that the function lives in Tools, it can be tested without creating anything on the system. Something like this in tools.test.ts:

it('escapeForShell', () => {
  expect(Tools.escapeForShell('$ABCD')).toBe('\\$ABCD');
  expect(Tools.escapeForShell('#$AB$C')).toBe('#\\$AB\\$C');
  expect(Tools.escapeForShell('ABCD')).toBe('ABCD');
});

These run instantly and pinpoint the problem if the escaping logic ever changes, while the memberResolve / objectResolve tests confirm it works end to end.

4. Small things

• In IBMiContent.ts there's an accidental formatting change (const inLocal =(s: string) plus trailing whitespace on the next line). Worth reverting to keep the diff clean.
• On a US CCSID variantChars.local is already $#@, so the # case ends up containing a $ too. That's fine since the leading character is what you're exercising, but good to be aware of.
• Existing examples in this repo: src/api/tests/suites/content.test.ts and src/api/tests/suites/tools.test.ts

Don't forget to tick the "have created one or more test cases" box in the PR description 🙂

[Nathaniel-King-Navarrete]

Hi @Nathaniel-King-Navarrete, thanks for adding tests, and for a first go they're in good shape: each one creates its own object, checks the result, and cleans up in a finally. A few suggestions:

1. Collapse the duplicated tests with it.each

The three memberResolve tests (and the three objectResolve ones) are identical apart from the name prefix. Vitest can run the same body over a list of values, so a new case becomes one extra entry in the array:

it.each(['$', '#', '#$'])('memberResolve with %s', async (prefix) => {
  const content = connection.getContent();
  const tempLib = connection.getConfig().tempLibrary;
  const tempSPF = `${prefix}ABCD`.concat(connection.variantChars.local);
  const tempMbr = tempSPF;

  try {
    const result = await connection.runCommand({
      command: `QSYS/CRTSRCPF ${tempLib}/${tempSPF} MBR(${tempMbr})`,
      environment: 'ile'
    });
    expect(result.code).toBe(0);

    const member = await content.memberResolve(tempMbr, [
      { library: 'QSYSINC', name: 'MIH' }, // Doesn't exist here
      { library: 'NOEXIST', name: 'SUP' }, // Doesn't exist here
      { library: tempLib, name: tempSPF } // Does exist here
    ]);

    expect(member).toEqual({
      asp: undefined,
      library: tempLib,
      file: tempSPF,
      name: tempMbr,
      extension: 'MBR',
      basename: `${tempMbr}.MBR`
    });
  }
  finally {
    await connection.runCommand({
      command: `QSYS/DLTF ${tempLib}/${tempSPF}`,
      environment: 'ile'
    });
  }
});

2. Assert that the setup worked

const result = await connection.runCommand(...) is assigned but never checked. If CRTSRCPF / CRTDTAARA fails (e.g. the object is left over from a previous run), the test fails later with a confusing "expected undefined to equal {...}". Adding expect(result.code).toBe(0) right after the setup (as in the example above) makes the real cause obvious.

3. Add a plain unit test for Tools.escapeForShell

Now that the function lives in Tools, it can be tested without creating anything on the system. Something like this in tools.test.ts:

it('escapeForShell', () => {
  expect(Tools.escapeForShell('$ABCD')).toBe('\\$ABCD');
  expect(Tools.escapeForShell('#$AB$C')).toBe('#\\$AB\\$C');
  expect(Tools.escapeForShell('ABCD')).toBe('ABCD');
});

These run instantly and pinpoint the problem if the escaping logic ever changes, while the memberResolve / objectResolve tests confirm it works end to end.

4. Small things

• In IBMiContent.ts there's an accidental formatting change (const inLocal =(s: string) plus trailing whitespace on the next line). Worth reverting to keep the diff clean.
• On a US CCSID variantChars.local is already $#@, so the # case ends up containing a $ too. That's fine since the leading character is what you're exercising, but good to be aware of.
• Existing examples in this repo: src/api/tests/suites/content.test.ts and src/api/tests/suites/tools.test.ts

Don't forget to tick the "have created one or more test cases" box in the PR description 🙂

@buzzia2001 Thank you for taking the time to give such generous feedback! I have made the requested changes, but I wasn't able to test them, since PUB400.com is getting transferred and that is what I use to test changes. Once I can test the changes, I will request a review. Thanks!

[buzzia2001]

Hi @Nathaniel-King-Navarrete, thanks for adding tests, and for a first go they're in good shape: each one creates its own object, checks the result, and cleans up in a finally. A few suggestions:

1. Collapse the duplicated tests with it.each

The three memberResolve tests (and the three objectResolve ones) are identical apart from the name prefix. Vitest can run the same body over a list of values, so a new case becomes one extra entry in the array:

it.each(['$', '#', '#$'])('memberResolve with %s', async (prefix) => {

const content = connection.getContent();

const tempLib = connection.getConfig().tempLibrary;

const tempSPF = ${prefix}ABCD.concat(connection.variantChars.local);

const tempMbr = tempSPF;

try {

const result = await connection.runCommand({

  command: `QSYS/CRTSRCPF ${tempLib}/${tempSPF} MBR(${tempMbr})`,

  environment: 'ile'

});

expect(result.code).toBe(0);

const member = await content.memberResolve(tempMbr, [

  { library: 'QSYSINC', name: 'MIH' }, // Doesn't exist here

  { library: 'NOEXIST', name: 'SUP' }, // Doesn't exist here

  { library: tempLib, name: tempSPF } // Does exist here

]);

expect(member).toEqual({

  asp: undefined,

  library: tempLib,

  file: tempSPF,

  name: tempMbr,

  extension: 'MBR',

  basename: `${tempMbr}.MBR`

});

}

finally {

await connection.runCommand({

  command: `QSYS/DLTF ${tempLib}/${tempSPF}`,

  environment: 'ile'

});

}

});

2. Assert that the setup worked

const result = await connection.runCommand(...) is assigned but never checked. If CRTSRCPF / CRTDTAARA fails (e.g. the object is left over from a previous run), the test fails later with a confusing "expected undefined to equal {...}". Adding expect(result.code).toBe(0) right after the setup (as in the example above) makes the real cause obvious.

3. Add a plain unit test for Tools.escapeForShell

Now that the function lives in Tools, it can be tested without creating anything on the system. Something like this in tools.test.ts:

it('escapeForShell', () => {

expect(Tools.escapeForShell('$ABCD')).toBe('\$ABCD');

expect(Tools.escapeForShell('#$AB$C')).toBe('#\$AB\$C');

expect(Tools.escapeForShell('ABCD')).toBe('ABCD');

});

These run instantly and pinpoint the problem if the escaping logic ever changes, while the memberResolve / objectResolve tests confirm it works end to end.

4. Small things

• In IBMiContent.ts there's an accidental formatting change (const inLocal =(s: string) plus trailing whitespace on the next line). Worth reverting to keep the diff clean.

• On a US CCSID variantChars.local is already $#@, so the # case ends up containing a $ too. That's fine since the leading character is what you're exercising, but good to be aware of.

• Existing examples in this repo: src/api/tests/suites/content.test.ts and src/api/tests/suites/tools.test.ts

Don't forget to tick the "have created one or more test cases" box in the PR description 🙂

@buzzia2001 Thank you for taking the time to give such generous feedback! I have made the requested changes, but I wasn't able to test them, since PUB400.com is getting transferred and that is what I use to test changes. Once I can test the changes, I will request a review. Thanks!

Perfect, please write me when it's ready!