Skip to content

Fix watch for packages when there are compilation errors in src/ #453

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.

Sign up for GitHub

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

Merged
harrysarson merged 2 commits into from
Oct 16, 2020
Merged

Fix watch for packages when there are compilation errors in src/ #453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72bf805
Fix watch for packages when there are compilation errors in src/
lydell Oct 15, 2020
2d40c8a
See if committing tests/fixtures/elm-home helps Travis
lydell Oct 15, 2020
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
11 changes: 2 additions & 9 deletions lib/Compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ function compile(

compileProcess.on('close', function (exitCode) {
if (exitCode !== 0) {
reject('Compilation failed');
reject(new Error(`\`elm make\` failed with exit code ${exitCode}.`));
} else {
resolve();
}
Expand Down Expand Up @@ -94,13 +94,7 @@ function compileSources(
if (exitCode === 0) {
resolve();
} else {
const msg =
'Compilation failed while attempting to ' +
(testFilePaths.length > 0
? 'build ' + testFilePaths.join(' ')
: 'run `elm make` on ' + projectRootDir);

reject(msg);
reject(new Error(`\`elm make\` failed with exit code ${exitCode}.`));
Copy link
Collaborator Author

@lydell lydell Oct 15, 2020

Choose a reason for hiding this comment

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

The old error was super verbose and not useful when the nice error from elm make is printed just above anyway.

}
});
});
Expand Down Expand Up @@ -138,7 +132,6 @@ function isMachineReadableReporter(reporter /*: string */) /*: boolean */ {

module.exports = {
compile,
compileSources,
compileAll,
getTestRootDir,
getGeneratedCodeDir,
Expand Down
13 changes: 3 additions & 10 deletions lib/Generate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,20 +52,13 @@ function readUtf8(filepath) {
function generateElmJson(
projectRootDir /*: string */,
generatedCodeDir /*: string */,
hasBeenGivenCustomGlobs /*: boolean */
hasBeenGivenCustomGlobs /*: boolean */,
elmJsonPath /*: string */,
projectElmJson /*: any */
) /*: [string, string, Array<string>] */ {
const testRootDir = Compile.getTestRootDir(projectRootDir);
const elmJsonPath = path.resolve(path.join(projectRootDir, 'elm.json'));
const generatedSrc = path.join(generatedCodeDir, 'src');

var projectElmJson = {};

try {
projectElmJson = fs.readJsonSync(elmJsonPath);
} catch (err) {
console.error('Error reading elm.json: ' + err);
process.exit(1);
}
var isPackageProject = projectElmJson.type === 'package';
var dependencies = Solve.get_dependencies(elmJsonPath);

Expand Down
205 changes: 59 additions & 146 deletions lib/elm-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,9 +230,9 @@ function runTests(generatedCodeDir /*: string */, testFile /*: string */) {
}
);
})
.catch(function (exitCode) {
.catch(function (error) {
console.error('Compilation failed for', testFile);
return Promise.reject(exitCode);
return Promise.reject(error);
});
}

Expand Down Expand Up @@ -281,7 +281,6 @@ function getGlobsToWatch(elmJson) {
});
}

let projectElmJson = {};
let report;

if (
Expand All @@ -305,77 +304,53 @@ function infoLog(msg) {
}
}

// It's important to globify all the arguments.
// On Bash 4.x (or zsh), if you give it a glob as its last argument, Bash
// translates that into a list of file paths. On bash 3.x it's just a string.
// Ergo, globify all the arguments we receive.
const isMake = args._[0] === 'make';
const testFileGlobs = isMake ? args._.slice(1) : args._;
const testFilePaths = resolveGlobs(testFileGlobs);
const projectRootDir = process.cwd();
const generatedCodeDir = Compile.getGeneratedCodeDir(projectRootDir);
const hasBeenGivenCustomGlobs = testFileGlobs.length > 0;

if (args._[0] === 'make') {
const files = args._.slice(1);
const elmJsonPath = path.resolve(path.join(projectRootDir, 'elm.json'));
let projectElmJson;

// It's important to globify all the arguments.
// On Bash 4.x (or zsh), if you give it a glob as its last argument, Bash
// translates that into a list of file paths. On bash 3.x it's just a string.
// Ergo, globify all the arguments we receive.
const fileGlobs = files.length > 0 ? files : [];
const testFilePaths = resolveGlobs(files);
const hasBeenGivenCustomGlobs = fileGlobs.length > 0;
const elmJsonPath = path.resolve(path.join(projectRootDir, 'elm.json'));
try {
projectElmJson = fs.readJsonSync(elmJsonPath);
} catch (err) {
console.error('Error reading elm.json: ' + err.message);
throw process.exit(1);
Copy link
Collaborator

@harrysarson harrysarson Oct 16, 2020

Choose a reason for hiding this comment

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

Do we have to crash if we are in watch mode and the project elm json is invalid?

Copy link
Collaborator

@harrysarson harrysarson Oct 16, 2020

Choose a reason for hiding this comment

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

And what happens if someone edits their elm.json while we are watching tests>

Copy link
Collaborator Author

@lydell lydell Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I didn’t think about that case! This could for sure be a future enhancement.

Currently this is done outside of the watch loop so it would require more changes than I’m comfortable with at the moment. This looks like added code but it’s in fact just moved so were keeping the same behavior as before regarding elm.json at least.

Currently, if elm.json changes while the tests run those changes are just ignored.

Copy link
Collaborator

@harrysarson harrysarson Oct 16, 2020

Choose a reason for hiding this comment

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

Before this PR generateElmJson read the elm.json file from disk everything. Would that mean that it would detect changes in your elm.json during watch mode?

Copy link
Collaborator Author

@lydell lydell Oct 16, 2020

Choose a reason for hiding this comment

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

Ah, good catch, didn’t think about that change. I’ll have to investigate.

Copy link
Collaborator Author

@lydell lydell Oct 16, 2020

Choose a reason for hiding this comment

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

Turns out Generate.generateElmJson is outside the watch loop too (and was before this PR as well). And moving it into the loop brings more issues:

  • We don’t watch elm.json so you need to edit some .elm file to trigger.
  • If you edit "source-directories" we need to recalculate which globs the watcher should look at, which is also outside the loop.
  • Reading elm.json and running elm-json currently does process.exit(1) on failure which would need to be refactored.
  • We basically need to restart the whole thing if elm.json changes.

My conclusion is:

  • This PR has not changed behavior.
  • Adding support for elm.json changes could be a nice future improvement.

Copy link
Collaborator

@harrysarson harrysarson Oct 16, 2020

Choose a reason for hiding this comment

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

My conclusion is:

  • This PR has not changed behavior.

Good enough for me!

}

try {
projectElmJson = fs.readJsonSync(elmJsonPath);
} catch (err) {
console.error('Error reading elm.json: ' + err);
process.exit(1);
}
const isPackageProject = projectElmJson.type === 'package';

const isPackageProject = projectElmJson.type === 'package';
if (isMake) {
Generate.generateElmJson(
projectRootDir,
generatedCodeDir,
hasBeenGivenCustomGlobs,
elmJsonPath,
projectElmJson
);

if (isPackageProject) {
// Run `elm make` on the project root, to generate the .elmi files.
Compile.compileSources(
[],
projectRootDir,
args.verbose,
pathToElmBinary,
args.report
)
.then(() =>
elmTestMake(
projectRootDir,
pathToElmBinary,
testFilePaths,
hasBeenGivenCustomGlobs
)
)
.catch((err) => {
console.error(err.message);
process.exit(1);
});
} else {
elmTestMake(
projectRootDir,
pathToElmBinary,
testFilePaths,
hasBeenGivenCustomGlobs
);
}
Compile.compileAll(
elmVersion,
testFilePaths,
generatedCodeDir,
args.verbose,
pathToElmBinary,
args.report
)
.then(function () {
process.exit(0);
})
.catch(function () {
process.exit(1);
});
} else {
// It's important to globify all the arguments.
// On Bash 4.x (or zsh), if you give it a glob as its last argument, Bash
// translates that into a list of file paths. On bash 3.x it's just a string.
// Ergo, globify all the arguments we receive.
const testFileGlobs = args._.length > 0 ? args._ : [];
const testFilePaths = resolveGlobs(testFileGlobs);

const elmJsonPath = path.resolve(path.join(projectRootDir, 'elm.json'));

try {
projectElmJson = fs.readJsonSync(elmJsonPath);
} catch (err) {
console.error('Error reading elm.json: ' + err);
process.exit(1);
}

const isPackageProject = projectElmJson.type === 'package';

if (testFilePaths.length === 0) {
var extraAppError =
"\n\nAlternatively, if your application has tests in a different directory, try calling elm-test with a glob: elm-test 'frontend-app/**/*Tests.elm'.";
Expand All @@ -392,37 +367,12 @@ if (args._[0] === 'make') {
process.exit(1);
}

if (isPackageProject) {
// Run `elm make` on the project root, to generate the .elmi files.
Compile.compileSources(
[],
projectRootDir,
args.verbose,
pathToElmBinary,
args.report
)
.then(() => generateAndRun(projectRootDir, testFileGlobs, testFilePaths))
.catch((err) => {
console.error(err.message);
process.exit(1);
});
} else {
generateAndRun(projectRootDir, testFileGlobs, testFilePaths);
}
}

function generateAndRun(
projectRootDir /*: string */,
testFileGlobs /* Array<string> */,
testFilePaths /*: Array<string> */
) {
const generatedCodeDir = Compile.getGeneratedCodeDir(projectRootDir);
const hasBeenGivenCustomGlobs = testFileGlobs.length > 0;

const returnValues = Generate.generateElmJson(
projectRootDir,
generatedCodeDir,
hasBeenGivenCustomGlobs
hasBeenGivenCustomGlobs,
elmJsonPath,
projectElmJson
);
const generatedSrc = returnValues[1];

Expand All @@ -438,27 +388,20 @@ function generateAndRun(
args.report
)
.then(function (testModules) {
return Promise.resolve()
.then(function () {
process.chdir(generatedCodeDir);

const mainFile = Generate.generateMainModule(
parseInt(args.fuzz),
parseInt(args.seed),
args.report,
testFileGlobs,
testFilePaths,
testModules,
generatedSrc,
processes
);

return runTests(generatedCodeDir, mainFile);
})
.catch(function (err) {
console.error(err.message);
process.exit(1);
});
process.chdir(generatedCodeDir);

const mainFile = Generate.generateMainModule(
parseInt(args.fuzz),
parseInt(args.seed),
args.report,
testFileGlobs,
testFilePaths,
testModules,
generatedSrc,
processes
);

return runTests(generatedCodeDir, mainFile);
})
.catch(function (err) {
console.error(err.message);
Expand Down Expand Up @@ -505,33 +448,3 @@ function generateAndRun(
});
}
}

function elmTestMake(
projectRootDir /*: string */,
pathToElmBinary /*: string */,
testFilePaths /*: Array<string> */,
hasBeenGivenCustomGlobs /*: boolean */
) {
const generatedCodeDir = Compile.getGeneratedCodeDir(projectRootDir);

Generate.generateElmJson(
projectRootDir,
generatedCodeDir,
hasBeenGivenCustomGlobs
);

Compile.compileAll(
elmVersion,
testFilePaths,
generatedCodeDir,
args.verbose,
pathToElmBinary,
args.report
)
.then(function () {
process.exit(0);
})
.catch(function () {
process.exit(1);
});
}
Binary file modified tests/fixtures/elm-home/0.19.1/packages/registry.dat
View file
Open in desktop
Binary file not shown.
Binary file modified tests/fixtures/elm-home/elm-json/versions.dat
View file
Open in desktop
Binary file not shown.