dataloaders tests #110
dataloaders tests #110
Conversation
src/dataloader.ts
Outdated
| function makeCommand(commandPath) { | ||
| if (commandPath.endsWith(".js")) return {command: "node", options: [commandPath]}; | ||
| if (commandPath.endsWith(".ts")) return {command: "tsx", options: [commandPath]}; | ||
| return {command: commandPath, options: []}; |
There was a problem hiding this comment.
should automatically assuming non .js/.ts files are executable or add a check for the .sh extension? We also haven't checked for compatibility with Windows and it should be something to consider before launch
not a blocker for this PR but something to think about
There was a problem hiding this comment.
Currently the data loaders are ignored all together if non executable. But I want to separate that issue (see #96 (comment)); the call failing because the script is not +x would just be one of the many possible errors anyway.
| @@ -0,0 +1,6 @@ | |||
| function mytest() { | |||
| // TODO: how to make sure this is not called by tsx? (and, does it matter?) | |||
There was a problem hiding this comment.
If you wanted to test this, you could test the error case: if you put TypeScript annotations in a .js file, you should get a syntax error. It’d probably be a good thing to test how runLoader handles errors anyway!
There was a problem hiding this comment.
I tried that but couldn't make it work, because tsx sends the syntax error anyway since the file extension is js.
(Note that STDERR is passed to the parent (for logging), with the "inherit" option.)
test:
it("a .js data loader is _not_ called with tsx", async () => {
const outputPath = ".observablehq/data4.txt";
await runLoader("test/dataloaders/data4-error.txt.js", outputPath);
try {
assert.strictEqual(await readFile(outputPath, "utf-8"), "");
} catch (error) {
assert.strictEqual(error.code, "ENOENT");
} finally {
unlink(outputPath);
}
});test/dataloaders/data4-error.txt.js
/* eslint-disable */
function mytest2(): string {
return "tsx";
}
console.log(mytest2());
|
I'll want to add the new tests to #114 at some point. |
|
I've edited this PR to just add tests for data loaders |
.gitignore
Outdated
| @@ -3,4 +3,5 @@ dist/ | |||
| docs/.observablehq/cache | |||
| node_modules/ | |||
| test/output/*-changed.* | |||
| test/.observablehq/cache | |||
There was a problem hiding this comment.
We shouldn’t be generating anything here now that #120 is merged. The data loaders were using the wrong cache directory. (We were erroneously using .observablehq/cache relative to the current working directory, rather than the intended source root.)
There was a problem hiding this comment.
even after merging #120 these tests are saved in test/.observablehq/cache/ (and I think that should be the intended source root for the tests—we don't want to interfere with docs?)
There was a problem hiding this comment.
Hmm, let me double-check what’s happening here. It may indeed be the case that the source root for the tests is the test folder.
There was a problem hiding this comment.
Ah, indeed you are passing in "test" as the first argument to Loader.load, so that makes sense. 👍
Co-authored-by: Mike Bostock <[email protected]>
PR edited to salvage the data loaders tests.
Previously:
Use node for .js loaders, tsx for .ts loaders
addresses #96
Note: as expected, this breaks existing loaders ending in .js or .ts and that expect to be called as (non-javascript) shell scripts. However node ignores the
#!line so if those loaders were already written in js or ts they should keep running.(The test for node is not great, in the sense that if the data1.js loader is called with tsx it works the same; I don't know how to distinguish these cases since in the end tsx is just a node script.)
This PR does not cover the executable mode issue.