Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,6 @@ jobs:
env:
NO_ELM_TOOLING_INSTALL: 1

- name: install glob 8
if: steps.cache-node_modules.outputs.cache-hit != 'true' && (matrix.node-version == '12.x' || matrix.node-version == '14.x')
run: npm install glob@8

- name: install mocha 9
if: steps.cache-node_modules.outputs.cache-hit != 'true' && (matrix.node-version == '12.x' || matrix.node-version == '14.x' || matrix.node-version == '16.x')
run: npm install mocha@9
Expand Down
41 changes: 18 additions & 23 deletions lib/FindTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const gracefulFs = require('graceful-fs');
const fs = require('fs');
const glob = require('glob');
const { globSync } = require('tinyglobby');
const path = require('path');
const Parser = require('./Parser');
const Project = require('./Project');
Expand Down Expand Up @@ -59,44 +59,39 @@ function resolveCliArgGlob(
path.resolve(fileGlob)
);

// glob@8 (via minimatch@5) had a breaking change where you _have_ to use
// forwards slash as path separator, regardless of platform, making it
// unambiguous which characters are separators and which are escapes. This
// restores the previous behavior, avoiding a breaking change in elm-test.
// The globs _have_ to use forwards slash as path separator, regardless of
// platform, making it unambiguous which characters are separators and which
// are escapes.
// Note: As far I can tell, escaping glob syntax has _never_ worked on
// Windows. In Elm, needing to escape glob syntax should be very rare, since
// Elm file paths must match the module name (letters only). So it’s probably
// more worth supporting `some\folder\*Test.elm` rather than escaping.
// https://github.com/isaacs/node-glob/issues/468
// https://github.com/isaacs/minimatch/commit/9104d8d175bdd8843338103be1401f80774d2a10#diff-f41746899d033115e03bebe4fbde76acf2de4bf261bfb221744808f4c8a286cf
const pattern =
process.platform === 'win32'
? globRelativeToProjectRoot.replace(/\\/g, '/')
: globRelativeToProjectRoot;

return glob
.sync(pattern, {
cwd: projectRootDir,
nocase: true,
absolute: true,
ignore: ignoredDirsGlobs,
// Match directories as well and mark them with a trailing slash.
nodir: false,
mark: true,
})
.flatMap((filePath) =>
filePath.endsWith('/') ? findAllElmFilesInDir(filePath) : filePath
);
return globSync(pattern, {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: there a big comment above about glob@8 and Windows and backslashes. Try it out on Windows to see how it behaves in tinyglobby, and remove or update the code/comment?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s needed: 7814ff7 (#647)

Updated the comment: 46ca4aa (#647)

cwd: projectRootDir,
caseSensitiveMatch: false,
absolute: true,
ignore: ignoredDirsGlobs,
// Match directories as well
onlyFiles: false,
}).flatMap((filePath) =>
// Directories have their path end with `/`
filePath.endsWith('/') ? findAllElmFilesInDir(filePath) : filePath
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: Does it end with a slash on Windows? We don’t seem to have test coverage for this condition being true.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test case: 36cdb47 (#647)

Btw, that test was failing on master – see #648. But it passed when installing glob 8 instead of 10. Another breakage by glob 🙈

);
}

// Recursively search for *.elm files.
function findAllElmFilesInDir(dir /*: string */) /*: Array<string> */ {
return glob.sync('**/*.elm', {
return globSync('**/*.elm', {
cwd: dir,
nocase: true,
caseSensitiveMatch: false,
absolute: true,
ignore: ignoredDirsGlobs,
nodir: true,
onlyFiles: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onlyFiles: true is the default, but I've kept it for explicitness.

});
}

Expand Down
Loading