Watcher: Cancel previous compilation and test runs #225
Labels
Comments
Merged
harrysarson
pushed a commit
that referenced
this issue
Dec 17, 2020
This makes the watcher faster and more stable! As a bonus, this made the code much more organized. - Concentrate all `process.exit` in elm-test.js (except one in Install.js where I found no clean solution). Besides making the code easier to follow, it allows the watcher to continue running (and recover) even if there are errors. - Don’t crash/stop running tests if files are removed. - Pick up new Elm files added. - Fixes #225 We already had code for waiting for the previous run to finish before starting the next run, but it queued up every single file change/add/remove event and ran that many runs one after the other. Now, for all watch events during a run we run _once_ after the current run for all of them. We also sleep 100 ms at the start of each run to catch more events happening close by (such as when adding/removing a whole folder of tests). (Note: We used to sleep at least 500 ms via the [awaitWriteFinish](https://github.com/paulmillr/chokidar#performance) chokidar option.) I’ve also tested manually that running elm-format on save doesn’t cause double test runs: #194 - Fixes #355 We now find the closest elm.json upwards in the folder hierarchy. - Resolves #453 (comment) The watcher now listens for changes to elm.json and properly recalculates things. I thought that I would have to recalculate some things only when elm.json changes for performance, but after making the globbing faster there was no need so we simply rebuild everything on each run. - Closes #463 and closes #464 I can’t find any more places to use async/await. - elm.json is now decoded properly in one place rather than here and there and just being `any`. We now have 0 explicit `any` type annotations! - elm-test.js now only contains CLI logic and subcommand dispatching. The code for running tests have been moved to RunTests.js and to FindTests.js (which used to be called Runner.js). - Improved .flowconfig that is stricter. Closes #478 - And lots of little code improvements along the way! I only touched things that I needed to change anyway in order to not make the diff balloon too much. I’ve done some manual testing on Windows, Mac and Linux to make sure things work like they should.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I opened #194 in the past and we took some steps in the right direction but I think the main issue is still existing:
node-test-runnerspawn's processes to compile elm sources, but doesn't cancel them if a new file change triggers a new run.Demo
This shows that touching a file frequently will lead to problems if the timespan is shorter than a complete test run including the compile step. In the example the test run itself isn't long, but you can see the overlapping runs and the cpu utilization. The issue get's worse for projects with a larger code base.
This issue happens in my work flow and I tend to disable the file watcher because its getting unusable. Instead I try to optimize and watch a single test file or start a test run manually.
I see there has been work done in the latest commits regarding the architecture where the supervisor and the workers communicate with each other. I'm not to sure what's going on in detail, so I'd like to discuss possible solutions to improve this issue. In my opinion it could work out to
killany spawned process if a new test run is triggered in watch mode. There is one part where the internal logic has to be able to be aborted and there is another that external processes (namely the elm compiler) have to be killed. I previously mentioned that it would be necessary to be able to stop the spawned process and migrate tocross-spawn-with-killor hope something is happening here moxystudio/node-cross-spawn#44 or use a different solution to control spawned processes (https://nodejs.org/api/child_process.html).My guess is also that the
node-elm-compilerhas to be adapted, to be cancelable. it contains also a dependency to[email protected].I'm happy to discuss if this solution to cancel spawned processes makes any sense at all. If there could be problems aborting the
elm-compilerand having an inconsistent state. Perhaps there are other options and debouncing/thottling the actual test runs will solve the issue.The text was updated successfully, but these errors were encountered: