Allow tests for shebang rule

[piranna]

shebang rule only check for the bin field in package.json, but with the new Node.js built-in test test runner, it's valid that they are run as standalone executables, with a shebang and an executable bit.

My suggestion is that files that import test or node:test can have a shebang, and if so, check and enforce that they have enabled the executable permission bit. In the other way could not work, importing test built-in and have enabled the executable permission bit is NOT enough to consider it should have a shebang, because Windows don't have permission bits and the file can be importing the test runner but not intended to be used standalone, except is the test script on package.json file is running the test file as if would be an executable, in that case file must have the shebang.

[scagood]

🤔 We could test if the file is executable?

Something like:

import { access, constants } from 'node:fs/promises';

export async function isExecutable(path) {
  try {
    await access(path, constants.X_OK);
    return true;
  } catch (error) {
    if (error.code === 'EACCES') {
      return false;
    }

    throw error;
  }
}

// Then just do this in the rule
if (hasShebang && !isExecutable) {
  context.report();
}

[piranna]

Yes, but that's not enough, we would also need to check for the import of test or node:test, and maybe also check if it's being run from the test script, both directly as an executable or as an argument of the test runner, or if filename has one of the names from the default test runner files.

[scagood]

Assuming { "type": "module" }, with the following test cases:

executable.js

#!/usr/bin/env node
console.info('executable')

test.js

#!/usr/bin/env node
import { equal } from 'node:assert/strict';
import { test } from 'node:test';

test('some test', () => {
  equal(true, true);
});

---

In that example:

• The only valid shebang is the one in executable.js.
• The shebang in the test.js file is invalid.

---

I do agree that the current rule is probably not right, but I think its for a different reason 👀

[piranna]

• The shebang in the test.js file is invalid.

The shebang is valid... if you add executable permissions :-) So the rule should check and fix it for the test.js file.

[scagood]

mmm, I see!

[scagood]

So, my thoughts on this are we could add the following:

{
  "n/shebang": [
    "error",
    // Defaults for the rule (mostly to prevent a breaking change)
    {
      // Leave the default check
      checkPackageBin: true,
      // Add the ability to enable test files
      checkTestFiles: false,
      // Check all executable files
      checkExecutable: false,
    }
  ]
}

For each of the options, if the following is matched we should check the shebang:

checkPackageBin:

1. Is the file referenced in the package.json#bin
2. Is the file executable - not sure about this one

checkTestFiles:

1. Check for ImportDeclaration, ImportExpression, and require calls
2. Check the file is executable.

checkExecutable:

1. Check if the file is executable

👨‍💻 A quick code example for checking for `node:test`, and `test`

I roughly validated this in https://astexplorer.net/

export default {
  create: function (context) {
    let foundTest = false;
    let fileExecutable = false;
    let validShebang = true;

    return {
      "Program:exit"(node) {
        if (foundTest === false) {
          return;
        }

        if (fileExecutable === false) {
          return;
        }

        if (validShebang === true) {
          return;
        }

        context.report({ node, message: "Program needs a valid shebang" });
      },

      /* import 'test'; */
      'ImportDeclaration[source.value="test"]'(node) {
        foundTest = true;
      },
      /* import 'node:test'; */
      'ImportDeclaration[source.value="node:test"]'(node) {
        foundTest = true;
      },
      /* await import('test'); */
      'ImportExpression[source.value="test"]'(node) {
        foundTest = true;
      },
      /* await import('node:test'); */
      'ImportExpression[source.value="node:test"]'(node) {
        foundTest = true;
      },
      /* require('test'); */
      'CallExpression[callee.name="require"][arguments.0.value="test"]'(node) {
        foundTest = true;
      },
      /* require('node:test'); */
      'CallExpression[callee.name="require"][arguments.0.value="node:test"]'(node) {
        foundTest = true;
      },

      /* Hacks for babel */

      /* await import('test'); */
      'CallExpression[callee.name="import"][arguments.0.value="test"]'(node) {
        foundTest = true;
      },
      /* await import('node:test'); */
      'CallExpression[callee.name="import"][arguments.0.value="node:test"]'(node) {
        foundTest = true;
      },
      /* await import('test'); */
      'CallExpression[callee.type="Import"][arguments.0.value="test"]'(node) {
        foundTest = true;
      },
      /* await import('node:test'); */
      'CallExpression[callee.type="Import"][arguments.0.value="node:test"]'(node) {
        foundTest = true;
      }
    };
  }
};

@aladdin-add What do you think?

[piranna]

checkPackageBin:

1. Is the file referenced in the package.json#bin
2. Is the file executable - not sure about this one

If file is in package.json#bin, it MUST has a shebang AND be executable.

checkTestFiles:

1. Check for ImportDeclaration, ImportExpression, and require calls
2. Check the file is executable.

Also 3. check it's run from package.json#scripts.test as an standalone executable

checkExecutable:

1. Check if the file is executable

Not sure what could be this checking...

[aladdin-add]

it's something like guessing the users' intent - IMHO, not all the tests are executable. i would suggest to adding a new option additionalExcutable (open to the option name 😄 ):

{
  "n/shebang": [
    "error",
    {
       "additionalExcutable": ["tests/*.js"]
    }
  ]
}

[piranna]

Exactly, not all tests would be executable, with executions bit and shebang, only the ones that are run directly as commands on the test script.

[piranna]

Somewhat related to this, I usually have some deploy scripts that I want them to be executable as part of my package.json file scripts object, but i DON'T want them to be included when publishing the package. I think that ignoring if a file has or not an executable bit if it's excluded by the files field or the .npmignore file would do the job too, in a more broad and generic way.

[scagood]

🤔 I think I like that approach.

[aladdin-add]

well after thinking for a while, I'd more prefer the way mentioned earlier: #119 (comment)

There is a difference that additionalExcutable is not ignoring; it's to enforce some additional files having the correct shebangs. Seems more in line with user expectations?

[aladdin-add]

@scagood wdyt?

[scagood]

We can add a couple of settings I think 🤔