Replace reading .elmi files with a faster and less hacky approach #442

lydell · 2020-09-27T14:35:10Z

Runs tests 0.5 seconds faster generally, the same speed or possibly a little bit slower in the worst (uncommon) case.
CI on this repo runs seems to run twice as fast!
Gets rid of the elmi-to-json binary. No longer need to rely on undocumented, internal binary formats.
On the other hand, we now have a custom Elm parser to maintain.
And rely on the internal JS output of the compiler. At least it feels like this depends on fewer implementation details than the .elmi approach.
More details: Replace reading .elmi files with a faster and less hacky approach #442 (comment)
Status: Stable enough for general use.

More about speed:

Both https://github.com/elm-in-elm/compiler/ and https://github.com/jfmengels/elm-review tests run 0.5 seconds faster on my machine.
@sporto reported that it worked fine and fast on his project as well, where elm-test 'src/**/*Test.elm' is used rather than tests/. https://discourse.elm-lang.org/t/do-you-put-tests-outside-the-tests-folder/6386/11
@bdukes reported it works fine and fast on Windows. https://discourse.elm-lang.org/t/do-you-put-tests-outside-the-tests-folder/6386/24
If you run elm-test src/, this PR results in a much bigger JS file (since we don’t know until runtime what is and isn’t a Test). In this uncommon case, the wins from faster compilation time is eaten by slower execution time. Can be improved by improving parallelization in the future.

Post merge follow up:

Make an issue about moving default seed generation to a much higher level of test generation. (~~ edited by @harrysarson)

Original PR description (outdated)

This is a proof of concept for @harrysarson’s idea to determine what exposed values are tests at runtime (using JavaScript) instead of at compile time (using elmi-to-json).

Pros:

No longer need to rely on undocumented, internal binary formats.
Should be a fix for requiring elmi-to-json is slow on Windows #385 (requiring elmi-to-json is slow on Windows)
Might be faster than compiling an extra time and then running elmi-to-json.

Cons:

Need to rely on the internal output JS. At least it feels like this depends on fewer implementation details than the .elmi approach.
Need an Elm code parser.
Might be slower than compiling an extra time and then running elmi-to-json.

TODO:

Replace the POC parser for finding which values a given .elm file exposes.
More tests.
Benchmark / Test in some real-world projects to see if it works out and if there’s any speed difference.

Notes:

elmi-to-json is still used to find the exact dependencies of package projects. This should be replaceable with elm-json solve. Leverage elm-json solve #356 adds elm-json, but not yet for finding exact dependencies of package projects.

.eslintrc.json

elm/src/Test/Runner/Node.elm

example-package-no-core/elm.json

example-package-no-tests/elm.json

lib/Generate.js

lib/Runner.js

lib/elm-test.js

tests/ci.js

lib/Parser.js

harrysarson · 2020-09-27T16:04:47Z

Thanks @lydell, this is really exciting! A couple of thoughts up front:

Realistically we will merge the elm-json PR before this (#356) so you will (after rebasing) be able to drop the dependency here.

This PR touches quite a bit of the test runners nasty internal machinery so we will have to be careful and also strike the right balance between not changing too much too reduce the risk of breakage and not changing enough that the existing troublesome parts of the test runner cause issues! (For example, we have had complaints that process.exiting causes corrupted output because the stream doesn't get time to flush, I see this PR adds more process.exit -- not a bad thing -- so we need to be careful that we don't make the problem worse.)

I will endeavour to review soon! Tagging @mpizenberg as I think this may be interesting to you :)

mpizenberg · 2020-09-27T16:25:56Z

Interesting indeed!

lydell · 2020-09-27T19:44:07Z

Ah, I see!

2 of the added process.exit are for validating the --compiler flag. That happens very early, before any streams or external processes have been started.

The remaining 2 are in places where we used to exit with “UnhandledPromiseRejection”. It’s unclear what happened before in those cases. But if anything I’d guess the new behavior is the same or at least not worse. Better handling with cleanup and stuff could be added in another PR.

harrysarson · 2020-09-27T21:30:53Z

That makes sense! Consider the process.exit concern resolved 👍

harrysarson · 2020-10-02T22:25:15Z

Bad news 😟

With this PR, we only find a fraction of the number of tests as without it. (I am testing on my copy of https://github.com/elm-in-elm/compiler/). Will investigate futher!

harrysarson · 2020-10-02T23:08:38Z

For reference this is my original sketch of this idea: https://gist.github.com/harrysarson/08acee2e8d8ffb0185b4b1020c08d756

Conflicts: lib/elm-test.js package.json tests/runner.js

Just like the no files found error.

lydell · 2020-10-24T12:44:35Z

Marking as “Ready for review” since I have no further ideas on things to improve (other than possibly adding a test for something – I’m all ears)! 🎉

lydell · 2020-10-24T12:47:34Z

elm/src/Test/Runner/Node.elm

+        model =
+            { available = Dict.empty
+            , runInfo =
+                { testCount = 0
+                , globs = []
+                , paths = []
+                , fuzzRuns = 0
+                , initialSeed = 0
+                }
+            , processes = 0
+            , nextTestToRun = 0
+            , results = []
+            , testReporter = createReporter report
+            , autoFail = Nothing
+            }


I’m not super happy about having to create an empty model that is never going to be used here, but it was the best I could come up with.

harrysarson

Some quick comments/questions. On the whole is very solid :)

I need to have a proper look through the parser still.

lib/Generate.js

Co-authored-by: Harry Sarson <[email protected]>

Instead we use the approach of <rtfeldman/node-test-runner#442> from which I have taken a lot of inspiration for this commit.

Fixes #211 Addresses #442 (comment) This PR makes a bunch of improvements to elm-test.js: - Properly validate CLI arguments: - Error on unknown flags. - Validate flag values early (seed and fuzz must be numbers, report must be console/json/junit). - Default flag values early, instead of in code generation or even in Elm. - elm-test.js is now 100% definitions, except the last line which is `main();` to kick things off. Previously, it was difficult to understand in what order things executed. There was some top-level code, a bunch of function definitions, some more top-level code, more functions, and so on. Now, you can follow the call stack from `main()`. Also, this means that there are way fewer global variables – functions now get passed what they depend on. - Use async/await where it helps (in preparation for merge conflicts with #464 – it should be possible to just pick this PR’s code for every conflict in elm-test.js). - Improved help message. Also, add some aliases, such as `-h`, for showing help. I find it super annoying when CLI tools make it difficult to find the help – which is the key to all other commands and flags. - Various cleanups. Help before: ``` Usage: elm-test init # Create example tests Usage: elm-test install PACKAGE # Like `elm install PACKAGE`, except it installs to "test-dependencies" in your elm.json Usage: elm-test TESTFILES # Run TESTFILES, for example tests/**/*.elm Usage: elm-test [--compiler /path/to/compiler] # Run tests Usage: elm-test [--seed integer] # Run with initial fuzzer seed Usage: elm-test [--fuzz integer] # Run with each fuzz test performing this many iterations Usage: elm-test [--report json, junit, or console (default)] # Print results to stdout in given format Usage: elm-test [--version] # Print version string and exit Usage: elm-test [--watch] # Run tests on file changes ``` Intermediate help (outdated): ``` elm-test init Create example tests elm-test Run tests in the tests/ folder elm-test TESTFILES Run TESTFILES, for example "src/**/*Tests.elm" elm-test install PACKAGE Like `elm install PACKAGE`, except it installs to "test-dependencies" in your elm.json Options: --compiler /path/to/compiler Use a custom path to an Elm executable. Default: elm --seed INT Run with a previous fuzzer seed. Default: A random seed --fuzz INT Run with each fuzz test performing this many iterations. Default: 100 --report json --report junit --report console Print results to stdout in the given format. Default: console --version Print version and exit --watch Run tests on file changes ``` [commander](https://github.com/tj/commander.js) help: ``` Usage: elm-test [options] [globs...] elm-test Run tests in the tests/ folder elm-test "src/**/*Tests.elm" Run tests in files matching the glob Options: --compiler <path> Use a custom path to an Elm executable (default: elm) --seed <int> Run with a previous fuzzer seed (default: 68917302767402) --fuzz <int> Run with each fuzz test performing this many iterations (default: 100) --report <json|junit|console> Print results to stdout in the given format (default: "console") --watch Run tests on file changes (default: false) --version Print version and exit -h, --help Show help Commands: init Create example tests install <package> Like `elm install package`, except it installs to "test-dependencies" in your elm.json make [globs...] Check files matching the globs for compilation errors help [command] Show help ```

lydell added 9 commits September 26, 2020 23:08

WIP use JS hack instead of elmi-to-json

3ec6e0e

Figured out how to handle module names/source directories

965a0fe

Simple case is working!

c41e4e9

Cleanups

38ffa98

Properly handle compile errors

5857c55

Fix package examples

ee168c2

Show more details when tests fail

3141bcc

Make tests pass

ad1aef3

Improve error messages

31cf530

lydell commented Sep 27, 2020

View reviewed changes

tests/ci.js Outdated Show resolved Hide resolved

lydell commented Sep 27, 2020

View reviewed changes

lib/Parser.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

harrysarson mentioned this pull request Sep 30, 2020

Avoid need for elmi-to-json on path mpizenberg/elm-test-rs#11

Closed

lydell added 9 commits October 3, 2020 14:22

Fix Flow errors

f9f7de2

WIP parser

da60e89

WIP parser

9c55636

Parse module declaration

55c1df8

Parse module declaration finished

99d9e6c

For-loop

d15b142

WIP pure functions

480a00f

Separate parser functions for the different stages

70e8c14

Parse the rest of the file

bce3f96

lydell mentioned this pull request Oct 24, 2020

Proposal: Independent semantic versioning #447

Closed

lydell added 5 commits October 24, 2020 11:39

Merge branch 'master' into js

67853e2

Conflicts: lib/elm-test.js package.json tests/runner.js

Nicer error when no tests found

6e13626

Better error messages when no tests found

6350f7e

Fix parser bug uncovered by the improved no Tests found reporting

607f8db

Print the no Tests found error to stderr

a773ff4

Just like the no files found error.

lydell marked this pull request as ready for review October 24, 2020 12:44

lydell commented Oct 24, 2020

View reviewed changes

lydell changed the title ~~Use a JS hack instead of reading .elmi files~~ Replace reading .elmi files with a faster and less hacky approach Oct 24, 2020

harrysarson added status: waiting for review Pull request needs a review and removed status: misc in progress We are actively working on this pull request. labels Oct 25, 2020

harrysarson reviewed Oct 26, 2020

View reviewed changes

lib/Generate.js Show resolved Hide resolved

lib/Generate.js Outdated Show resolved Hide resolved

lib/Generate.js Show resolved Hide resolved

lib/Generate.js Show resolved Hide resolved

lib/Generate.js Outdated Show resolved Hide resolved

lib/Generate.js Show resolved Hide resolved

Rename makeReportCode

d7c3600

Co-authored-by: Harry Sarson <[email protected]>

lydell force-pushed the js branch from 77e99f9 to d7c3600 Compare October 26, 2020 19:39

lydell added 8 commits October 27, 2020 18:18

Rename hackCompiledElmJs

29b1c81

Clean up Parser mutation code

90f41ef

Remove StopParsing class

9a73a43

Use objects instead of error classes

0675fd0

Improve parse error logging

a8c2147

Merge branch 'master' into js

cd72ef8

Further simplifications

4793491

Don’t discard first flush results if several cmds

74f193d

harrysarson merged commit 4771b8c into rtfeldman:master Oct 28, 2020

lydell deleted the js branch October 28, 2020 20:39

lydell mentioned this pull request Oct 30, 2020

Improve elm-test.js #465

Merged

harrysarson added a commit to harrysarson/elm-test-rs that referenced this pull request Oct 31, 2020

drop dep on elmi on path

ba6b277

Instead we use the approach of <rtfeldman/node-test-runner#442> from which I have taken a lot of inspiration for this commit.

harrysarson mentioned this pull request Oct 31, 2020

No elmi mpizenberg/elm-test-rs#15

Merged

3 tasks

lydell mentioned this pull request Nov 5, 2020

Received invalid JSON from test interface search #258

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace reading .elmi files with a faster and less hacky approach #442

Replace reading .elmi files with a faster and less hacky approach #442

lydell commented Sep 27, 2020 •

edited

Loading

harrysarson commented Sep 27, 2020 •

edited

Loading

This comment has been minimized.

mpizenberg commented Sep 27, 2020

This comment has been minimized.

lydell commented Sep 27, 2020

harrysarson commented Sep 27, 2020

harrysarson commented Oct 2, 2020

harrysarson commented Oct 2, 2020

lydell commented Oct 24, 2020

lydell Oct 24, 2020

harrysarson left a comment

Replace reading .elmi files with a faster and less hacky approach #442

Replace reading .elmi files with a faster and less hacky approach #442

Conversation

lydell commented Sep 27, 2020 • edited Loading

harrysarson commented Sep 27, 2020 • edited Loading

This comment has been minimized.

mpizenberg commented Sep 27, 2020

This comment has been minimized.

lydell commented Sep 27, 2020

harrysarson commented Sep 27, 2020

harrysarson commented Oct 2, 2020

harrysarson commented Oct 2, 2020

lydell commented Oct 24, 2020

lydell Oct 24, 2020

Choose a reason for hiding this comment

harrysarson left a comment

Choose a reason for hiding this comment

lydell commented Sep 27, 2020 •

edited

Loading

harrysarson commented Sep 27, 2020 •

edited

Loading