highlightjs / highlight.js

JavaScript syntax highlighter with language auto-detection and zero dependencies.
https://highlightjs.org/
BSD 3-Clause "New" or "Revised" License
23.31k stars 3.52k forks source link

Autogenerated .DS_Store files on macOS are included in detect test suite #3973

Open GrygrFlzr opened 5 months ago

GrygrFlzr commented 5 months ago

Summary macOS occasionally generates .DS_Store files to store metadata about a directory for its builtin Finder application. Unfortunately, this can sometimes happen in a test/detect directory and generate erroneous detect test cases. Once the file exists, it's pretty hard to permanently remove it, as macOS will almost instantly regenerate the file if it is manually removed by rm -f .DS_Store or by other means.

Reproduction Steps Unfortunately, I cannot seem to reliably trigger the heuristic that generates the .DS_Store file, so instead one can emulate the behavior by doing the following:

  1. Clone the highlight.js repository
  2. Run echo '[hello]' > test/detect/1c/.DS_Store from the repository root
  3. Build with node ./tools/build.js -t node
  4. Run the full test suite with npm run test
  5. Get a failing test reporting AssertionError: .DS_Store should be detected as 1c, but was angelscript

Expected behavior Since the user has little control over .DS_Store files, skip .DS_Store files when iterating through the directory here: https://github.com/highlightjs/highlight.js/blob/e964bec6c8d72d263042ab37ae196baa79f49c47/test/detect/index.js#L20-L22

Additional context There are several ways to approach the problem: __1. Indiscriminately skip all .DS_Store files__

  const filenames = await fs.readdir(languagePath); 
  await Promise.all(filenames 
+   .filter(filename => filename !== '.DS_Store')
    .map(async function(example) { 

This is the easiest to implement, effectively hardcoding a blocklist of .DS_Store files. If someone decided to make a language that used the .DS_Store extension, they can still just name the detect test file something like testcase.txt, since the current test runner does not care about file extensions.

__2. Only skip all .DS_Store files on macOS__

  const filenames = await fs.readdir(languagePath); 
  await Promise.all(filenames 
+   .filter(filename => process.platform !== "darwin" || filename !== '.DS_Store')
    .map(async function(example) { 

This does not skip .DS_Store on other platforms. You could also optimize it into a one-time check for other platforms:

+ const checkDsStoreOrNoOp =
+   process.platform !== 'darwin'
+    : (_) => true
+    ? (filename) => filename !== '.DS_Store';
...
  const filenames = await fs.readdir(languagePath); 
  await Promise.all(filenames 
+   .filter(checkDsStoreOrNoOp)
    .map(async function(example) { 

The main drawback is that it technically causes inconsistent behavior between platforms (where the test only ignores the file on macOS), so the indiscriminate block suggested in (1) may be preferable.

3. Do nothing This kinda sucks but since nobody reported this before I guess it's not a very common issue.

joshgoebel commented 5 months ago

Never seen this [wrt Highlight.js] in all my years of maintainer-ship (on Mac OS X). That said an indiscriminate filter would be fine. It would also need to be applied to the checkAutoDetect.js script... a PR would be welcome.

dschach commented 4 months ago

I usually put .DS_Store in my .gitignore file and delete all the files in the repo. They won't come back

Edit: It's already in the .gitignore folder, so deleting the files will be fine if they are there.

joshgoebel commented 3 months ago

Would anyone here like to pick up this issue and build a fix?