Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
GrygrFlzr opened this issue Jan 21, 2024 · 5 comments
Open
Assignees
Labels
bug good first issue Should be easier for first time contributors help welcome Could use help from community parser

Comments

@GrygrFlzr
Copy link

GrygrFlzr commented Jan 21, 2024

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:

const filenames = await fs.readdir(languagePath);
await Promise.all(filenames
.map(async function(example) {

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 GrygrFlzr added bug help welcome Could use help from community parser labels Jan 21, 2024
@joshgoebel
Copy link
Member

joshgoebel commented Jan 21, 2024

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 joshgoebel added the good first issue Should be easier for first time contributors label Jan 21, 2024
@dschach
Copy link
Contributor

dschach commented Feb 19, 2024

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
Copy link
Member

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

@shirsakm
Copy link

I would like to work on this issue, is it open?

@joshgoebel
Copy link
Member

Sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Should be easier for first time contributors help welcome Could use help from community parser
Projects
None yet
Development

No branches or pull requests

4 participants