highlight.js icon indicating copy to clipboard operation
highlight.js copied to clipboard

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

Open GrygrFlzr opened this issue 1 year ago • 5 comments

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.

GrygrFlzr avatar Jan 21 '24 12:01 GrygrFlzr

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.

joshgoebel avatar Jan 21 '24 19:01 joshgoebel

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.

dschach avatar Feb 19 '24 05:02 dschach

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

joshgoebel avatar Mar 19 '24 03:03 joshgoebel