-
Notifications
You must be signed in to change notification settings - Fork 22
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
test: add comprehensive unit tests for model parse, analysis, and code gen #381
Conversation
…e general entry point for all tests
…d account for subscripts model test changes
…DIRECT LOOKUPS and subscripts
@@ -3,17 +3,17 @@ | |||
"version": "0.7.9", | |||
"description": "The core Vensim to C compiler for the SDEverywhere tool suite.", | |||
"type": "module", | |||
"files": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This files
section is effectively replaced by the .npmignore
file, which is the only reliable way to filter out things like test files that we don't want in the published package.
export let strings = [] | ||
|
||
// XXX: This is needed for tests due to sequence numbers being in module-level storage | ||
export function resetHelperState() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added these reset functions for now. Ideally we wouldn't rely on module-level (almost global-like) storage since it makes things harder to test. But that's a bigger change that I didn't want to address here, so for now the reset functions will serve the purpose.
@@ -200,15 +209,6 @@ export let isIterable = obj => { | |||
} | |||
return typeof obj[Symbol.iterator] === 'function' | |||
} | |||
export let stringToId = str => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was a holdover from the old web app generator, no longer used anywhere, so I deleted it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is used to declare some types and functions that make it easier to define the "expected" state that is verified in the tests. I didn't spend time making it very pretty or well documented since it's only to support the tests.
* Note that this function currently does not return anything and instead stores the parsed subscript | ||
* definitions in the `subscript` module and the parsed/analyzed variables in this module. | ||
* | ||
* @param {import('../parse/parser.js').VensimModelParseTree} parseTree The Vensim parse tree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of a place where I started defining some types via JSDoc. I only added type info where it was straightforward, and for other cases I left it as *
(wildcard / any). I figure it's good enough for now (not really the main focus of this PR) and something we can improve over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like adding JSDoc for documentation purposes. If we get type checking out of it too, that's great. I haven't found much need for type checking in SDEverywhere development, as the data structures and class hierarchy aren't that complicated. I do forget what function arguments mean, though, so documenting them would be helpful to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be great to have lower-level tests. This approach looks fine to me. Using JSDoc comments for type safety will be a lot less work than converting the compiler to Typescript.
Fixes #356
See issue for the rationale for adding these tests.
Overview
There are 4 big test files added to cover the following phases. I chose these specifically because they are the parts of the pipeline that currently have a direct dependency on antlr4-vensim (and those are the phases I'm working on refactoring to use an AST so as to not have that direct Vensim format dependency).
Each test file has two main parts. The first part has the more fine-grained tests that focus on a particular kind of subscript definition or expression or function call. The second part runs tests on the sample models.
It's possible that the latter tests are a bit redundant (since my hope is for the fine-grained tests to achieve 100% coverage on their own), but running the tests against the sample models is good as integration-style testing (to cover more real world models) and to cover combinations of things that might not already be covered by the fine-grained tests.
For each test, we run the compiler up until the end of that particular phase, then verify the state of the internal representation.
These are the tests and the phases they cover:
read-subscripts.spec.ts
This was the only place where I made minor modifications to the code to make it more testable. I split the existing
Model.readSubscriptRanges
into two functions:readSubscriptRanges
usesSubscriptRangeReader
to read in the parsed subscript range definitions, but doesn't process them in any way.resolveSubscriptRanges
processes those parsed subscript definitions from the previous step.You can see from the
Model.js
diff that I didn't change any of the logic, only split it into two parts so that the two phases could be independently tested.read-variables.spec.ts
This covers
Model.readVariables
andVariableReader
. This phase generatesVariable
instances but doesn't do much analysis or resolving (that happens in the next step).read-equations.spec.ts
This covers
Model.analyze
(which includesreadEquations
andEquationReader
). This phase processes theVariable
instances from the previous one, fills in details, generates internal variables, etc.gen-equation-c.spec.ts
This covers
EquationGen
. These tests specifically verify Vensim -> C code generation.Notes
I will add some PR comments to help annotate specific changes, but here are some general notes:
There are no changes to the behavior of the
compile
package.These aren't exactly "unit" tests. More like something between unit and integration tests, and also a bit like snapshot tests. But regardless of what we call them, they're more fine-grained than the existing C integration tests, and run in a fraction of the time (< 1 second). There's value in both kinds of tests. The existing C integration tests are still very valuable in verifying the overall model behavior. And the new tests allow for more easily zooming in on a particular part of the compiler pipeline.
We're using Vitest to run the tests, just like in all the other SDE packages.
I've added some JSDoc-style comments to the functions being tested, partially to better document the behavior of the existing code, but also to gradually introduce some type checking as an optional build-time step. (I have no interest in converting the
compile
package to full-on TypeScript, so this is an OK middle ground.)The tests themselves are written in TypeScript to help provide some type safety around the test helper functions. But this has no bearing on the rest of the package. The library itself is still pure JS and doesn't require any compilation step. (The
package.json
has been updated so that test files are excluded from the published package.)For the "expected" code, I didn't write this all out manually, because that would take forever. Instead, I wrote a helper function (see
logPrettyVars
) that writes out a terse representation of the expected state for a particularVariable
instance. My process was, for each test:logPrettyVars
to generate the expected outputexpect
clause of the test