Openfn compile for unit tests#1443
Conversation
|
@mtuchi I'm not liking the When you do When you do When you do I'd also suggest that So drop the |
|
QA walk through video for @hunterachieng |
- Add --test flag: compiles job expressions for unit testing, writing output to tests/ by default (reads dirs.tests from openfn.yaml) - Add --strip (default on with --test): tree-shakes compiled output, keeping only export const/function declarations and their transitive dependencies; use --no-strip to keep all compiled code - Add --watch flag: watches source files and recompiles on change (uses chokidar) - Strip mode removes injected _defer import when operations are stripped - Skip writing files with no exportable code after stripping; log when skipping - Auto-clean stale step files in tests/ that were skipped in the current run without touching user-added files at other paths - Fix --test --no-strip skipping pure-operation jobs (hasExportableCode guard now only applies when strip is active) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- remove export default [] from strip mode output (not needed for unit testing) - add --no-strip flag to keep full compiled output including operations - remove --strip standalone flag (stripping is always on with --test by default) - auto-derive output path for single-file --test using tests/ dir - skip writing files with no exportable code in strip mode - auto-clean stale step files in tests/ after project-wide strip runs - fix --test --no-strip skipping pure-operation jobs - update help examples and option descriptions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…mpile - Drop --test and --strip/--no-strip flags from openfn compile - Add --exports-only flag (opt-in) to strip operation calls, keeping only exported constants and functions for unit testing - openfn compile (no args) now compiles the whole project to compiled/ by default - openfn compile <workflow-name> looks up a workflow by name/id and compiles it - Extract stripping logic into a new exports-only transformer (order: 0) that runs before all others; remove strip option from top-level-operations - Update unit-testing-jobs.md to document the new workflow Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rce strings - Strip exports-only transformer down to a simple filter (imports + named exports only) — removes collectRefs/buildDeclMap/collectDeps dep-tracking - Rewrite exports-only tests using before/after source strings instead of AST builders - Extract runCompile helper in handler.ts to remove duplicated watch/non-watch logic - Clean up (step as any) casts in compile.ts now that step types are correct - Update unit-testing-jobs.md to drop the non-exported dependency note Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ject loop - hasExportableCode now lives in exports-only.ts and is exported from @openfn/compiler — the compiler decides what's exportable, not the CLI - Simplified regex: only match explicit export declarations (non-exported helpers are stripped by exports-only, so they'll never appear in the output) - Flatten nested workflow/step loops into a flat allSteps array in compileProject - Downgrade skipped-step log from info to debug - Remove hasExportableCode tests from CLI (now covered in compiler's exports-only test suite) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Just skip steps with no exportable code — don't track or delete old output files. The compiled dir is a dist folder; cleanup can be a future --clean flag if needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Unit test are failing, even on main. I am not sure what's causing it, i will investigate more tomorrow |
|
@mtuchi the tests failing here in CI are probably just flaky ones. I'll re-run them now. Locally it's probably most important that your compiler tests are passing. Sometimes my CLI tests fail locally because of caches stuff (they take so long to test I don't tend to run the whole suite locally - just the bit I'm working on) |
|
sorry @mtuchi I don't think I'm going to get to this this week. I've not forgotten, just couldn't get to it yet. It'll be high priority early next week. |
josephjclark
left a comment
There was a problem hiding this comment.
Hi @mtuchi, I'm so sorry for the slow reaction on this.
I've been through and tested and it all works great. I've picked out a few things that need addressing - mostly style refactors but also a couple of important details.
If you don't have time to resolve this I may be able to pick it up at the end of the week.
| o.repoDir, | ||
| o.trace, | ||
| o.useAdaptorsMonorepo, | ||
| o.watchFlag, |
There was a problem hiding this comment.
We need to add the workspace command now: this sets the working directory for the current project. --workspace is used by openfn execute and openfn project.
Very very useful for local testing!
| }; | ||
|
|
||
| const runCompile = async (options: CompileOptions, logger: Logger) => { | ||
| if (options.workflowName) { |
There was a problem hiding this comment.
The logic here feels weird. workflowName feels too important, and compileProject(options, logger) is duplicated, which is a smell.
A cleaner entrypoint for this stuff is good. Basically if the path does not end with a file extension, we should compileProject
| ? null | ||
| : path.resolve( | ||
| cwd, | ||
| opts.outputPath ?? wsConfig.dirs?.compiled ?? 'compiled' |
There was a problem hiding this comment.
I would rather compile to dist than compiled. That's more likely to be tracked by a gitignore file too.
| ? null | ||
| : path.resolve( | ||
| cwd, | ||
| opts.outputPath ?? wsConfig.dirs?.compiled ?? 'compiled' |
There was a problem hiding this comment.
also wsConfig.dirs.compiled feels like a new key in workspace config. It should be defined in lexicon/core.d.ts:WorkspaceConfig
| if (opts.outputStdout) { | ||
| log.success(`// ${stepId}\n\n` + code); | ||
| } else { | ||
| const outPath = path.join(compiledDir!, workflow.id, `${step.id}.js`); |
There was a problem hiding this comment.
It's always bothered me that job.js and job.compiled.js have the same file extension. For years I'd wondered if we should be using job.ojs for job expressions - but that ship has long since sailed.
As a compromise, can we output compiled files to job.mjs? That's more standard for modern Javascript esm modules, and it helps to distunguish a bit the file types
| import type { Transformer } from '../transform'; | ||
|
|
||
| // Returns true if compiled output contains any declarations worth importing in tests. | ||
| export const hasExportableCode = (code: string): boolean => |
There was a problem hiding this comment.
This would break with export { x, y, z } no?
Not a fan of this. What purpose does it solve? Feels like we're over-optimising
| const compile = (source: string, options = {}) => | ||
| print(transform(parse(source), [visitors], options)).code.trim(); | ||
|
|
||
| // --- hasExportableCode --- |
There was a problem hiding this comment.
This comment has no value. AI is terrible fof this sort of thing - please try and be strict on it. Only keep stuff with value
| test('is a no-op when options is not true', (t) => { | ||
| const before = `fn(); | ||
| export default [];`; | ||
| t.is(compile(before), before); |
There was a problem hiding this comment.
I don't like logic inside an assertion. A better pattern is:
const before = `fn();
export default [];`;
const after = compile(before)
t.is(after, before);
| t.false(hasExportableCode('fn();\nget();')); | ||
| }); | ||
|
|
||
| test('is a no-op when options is not true', (t) => { |
There was a problem hiding this comment.
In terms of storytelling through tests, rather than testing the inverse case first, I'd rather test basic functionality:
strips a non exported value
is a no-op if exports-only is false
Where the "before" code in both tests is exactly the same
|
|
||
| test('is a no-op when options is false', (t) => { | ||
| const before = `fn();`; | ||
| t.is(compile(before, { 'exports-only': false }), before); |
There was a problem hiding this comment.
Actually come to think of it, I don't like the use of compile here
The other transformer functions just test the transform functoin with visitors passed in.
This calls the top level compile function, so it's testing all sorts of logic before even getting to the transformer.
Ask the AI to rewrite these tests against the transform function, not the compile function. use the parse function with source code code, like in lazy-state.test.ts, rather than the the AST builders in add-imports.test.ts
- Add --clean to remove the output folder before compiling (off by default) - Add --workspace support, as in execute and project commands - Default output dir is now dist/ (or dirs.compiled from openfn.yaml) - Compiled files use the .mjs extension - Simplify runCompile routing: file paths compile directly, anything else compiles the checked-out project - Watch mode respects the configured workflows dir and workspace root, and ignores the resolved output dir Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Type the compiled output dir key used by openfn compile. buildConfig passes it through without a default so it is only serialized into openfn.yaml when the user sets it; the CLI defaults it to dist/ at point of use. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Short Description
Update
openfn compileto compiles workflows job expressions to standard JavaScript, writing output tocompiled/by default.Fixes #1424
Implementation Details
openfn compilenow compiles all workflows in the project tocompiled/by default (no flag needed)openfn compile <workflow-name>looks up a workflow by name in the project and compiles it tocompiled/use -Ofor stdout or-o <dir>to redirect outputopenfn compile <file.js>and openfn compile<workflow.json>retain their existing behaviour (stdout by default)Added new exports-only transformer (
src/transforms/exports-only.ts) with a single responsibility: keep imports, named export declarations, and their transitive non-exported dependencies; drop everything else (operations, export default, bare declarations). Runs at order: 0 (beforeall other transformers). Is a no-op unless explicitly enabled.
Added
--exports-onlyflag: strips adaptor operation calls and keeps only exported declarations — intended for unit testing job codeQA Notes
Setup
Check out the
1424-unit-testsbranch and installopenfnx:Verify the correct version is installed —
openfnx --versionshould outputbranch/1424-unit-tests.Testing
openfn compileNavigate to a local OpenFn project containing an
openfn.yamlfile and test the newopenfn compilecommand. Available options:AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy