Feature request: integrate elm-test

ymtszw commented 5 years ago

It would be really cool if elmjutsu integrate elm-test, especially, handling compilation results of files under tests/.

Currently, on-save compilation of elmjutsu (the feature migrated from linter-elm-make) cannot compile files under tests/, and showing an error like this: ... which is understandable, since in elm-test, files under tests/ are considered separate Elm project, and there are many missing parts required (which are prepared internally by node-test-runner) to properly compile test suites project.

(The error itself is resolvable if we set "main path" for compilation as suggested in https://github.com/halohalospecial/atom-elmjutsu/issues/130#issuecomment-418139691, though this workaround just tells elmjutsu to NOT compile files under tests/ even if the currently worked-on file is a test file, if I understand correctly)

Actually I suspect this "problem" is carried over from linter-elm-make seeing this seemingly-related issue: https://github.com/mybuddymichael/linter-elm-make/issues/115 Although I do not recall exact bahaviors of linter-elm-make when test files are saved. Anyway, in Elm 0.19, many things have changed like usage of elm-stuff/ and structure of elm.json, so they may not directly relate.

The root issue is: "test-dependencies" are now included in projects' elm.json, yet elm compiler itself does not simply compile tests/* files and require aid from node-test-runner. So at least until elm-test is officially integrated in elm, we have to communicate with elm-test in order to achieve the goal. (I could be wrong here, there may be simpler ways to compile tests/* files just for error checking in editor)

However, currently elm-test does not have an option to compile test project with elm make --report=json in order to acquire editor-plugin-friendly compilation results. (There is --report option though it is only emitting test-related events in JSON, not compilation results) So the work may have to start from contributing to elm-test. Also, perhaps we want "just compile and not actually run tests" option in elm-test, since tests themselves could take long time than compilation.

Works required looks quite large so I cannot just demand quick action, though I would be really happy to hear from folks about this.

halohalospecial commented 5 years ago

Hi @ymtszw, thanks for the great inputs!

Indeed, elm-test does not return Elm Make error messages in JSON format, only the test results. We're currently blocked on that. I'm not sure, but maybe it's better to have https://github.com/mbuscemi/elm-test-runner handle elm-test-related features? Unless we can gain something from integrating them in Elmjutsu.

MethodGrab commented 5 years ago

The bad import: import Test error can be easily fixed by moving elm-explorations/test from test-dependencies to dependencies in elm.json. After that, linting works fine in the tests.

Is it possible for elmjutsu to grab all the direct and indirect test-dependencies and internally add them to dependencies before running elm make?

ymtszw commented 5 years ago

@halohalospecial Separating responsibility sounds ok as long as elm-test-runner is active in 0.19, but it seems elm-test-runner also does not report compilation results in linter style is it? (Again it is understandable since elm-test cannot return elm make results in JSON)

@MethodGrab Thanks for the head up! That workaround is actually handy at least in application type project, since dependencies that aren't actually used are not bundled in built artifact, so putting elm-explorations/test in dependencies does not have significant drawback. Although for package type applications it cannnot be that way, so if elmjutsu can

grab all the direct and indirect test-dependencies and internally add them to dependencies before running elm make

it might be ok for the mean time.

Though even with this, If you have "always compile main" active, it still does not compile test files on save. So it would be even nicer if elmjutsu treats files under tests/ differently when the option is active.

halohalospecial commented 5 years ago

@ymtszw, you can set more than one main path using Elmjutsu: Set Main Paths (separated by comma) :slightly_smiling_face:

For example, you can set it to src/Main.elm, tests/Tests.elm.

ymtszw commented 5 years ago

@halohalospecial I thought I could live with that, but found an oddity, could make a new issue:

(in Windows 10, Elmjutsu 8.6.2)
When "always compile main" active, indirectly found errors (errors found in files that are not ones specified as "main paths") are not marked/shown inline (they ARE counted in the linter indicator in status bar). Like so:
- Here, src/Main.elm is the "main path", and there's error in src/Data/UniqueId.elm (which is reachable from src/Main.elm)
  - As a side note: main paths specified in Unix style (src/Main.elm) also works in Windows, which is quite helpful 👍
- If errors are directly found in "main path" files, they are correctly shown inline, regardless of files from which compilations are triggered
- I do not recall when this behavior starts, since in linter-elm-make, indirect errors were always shown, IIRC

halohalospecial commented 5 years ago

@ymtszw, thanks a lot for reporting the issue! I'm currently not using Elm and didn't realize that paths returned by elm make are sometimes absolute and sometimes relative. Can you check out v8.6.3? Thanks!

ymtszw commented 5 years ago

@halohalospecial Thanks, the problem looks resolved ❤️

So, regarding to test file compilation in Elmjutsu, current solution could be:

Include elm-explorations/test in dependencies rather than test-dependencies. This allows Elmjutsu to compile test files with just elm make without help of elm-test
Activate Always Compile Main option and add test files and application's main path as "main paths"

A caveat is, in package type project we should not include elm-explorations/test in dependencies. In applicaiton type project we can live with this method.

ymtszw commented 5 years ago

From slack: https://elmlang.slack.com/archives/C0SP19AMA/p1536980552000100

rtfeldman [1 day ago] elm-test make is out in the current elm-test@beta for 0.19, which makes it possible to compile files in tests/ as normal, automatically using test-dependencies in elm.json appropriately.

rtfeldman [1 day ago] more info here: https://github.com/rtfeldman/node-test-runner/issues/284

So now editor plugins can compile Elm files under tests/ with ease, if appropriate version of elm-test installed. I think this is a way worth exploring!

halohalospecial commented 5 years ago

Hi @ymtszw, have you tried elm-test make? I tried elm-test make --report json, but the output is not in JSON when there's an error.

halohalospecial commented 5 years ago

Oh, you reported it already :smile_cat:

https://github.com/rtfeldman/node-test-runner/issues/293

rtfeldman commented 5 years ago

FYI, this has been fixed in master - it now passes through --report=json to elm make.

I'm still working on a couple of bugfixes before the next release, but it should be included in beta10!

halohalospecial commented 5 years ago

Hi @ymtszw, @rtfeldman,

Can you check out Elmjutsu v9.0.0? Thanks!

ymtszw commented 5 years ago

Thanks for the update! Some issues:

(Windows 10, nodejs 8.12.0, Yarn 1.10.1, elm-test path at %USER%\AppData\Local\Yarn\bin\elm-test)

Better tell in doc that it requires elm-test@0.19-beta10 or later (until it's published, it requires node-test-runner@master, though)
It seems it's not working without Always Compile Main? While the option disabled, it shows this:
- Both in "type": "application" and "type":"package" projects
- When the option is enabled, it compiles test files correctly in both projects.
Completions in test files are working for "type":"application" projects. Though in "type":"package" projects, there is a subtle awkwardness:
- it works for modules in that package and ones being depended on (say, modules in elm/core and other packages in "dependencies")
- does NOT work for test-related modules, like Test.describe (possibly, modules comes via packages derived from "test-dependencies")
Side note: Now the workarounds described in https://github.com/halohalospecial/atom-elmjutsu/issues/131#issuecomment-419693056 are no longer required. elm-explorations/test can now just live in test-dependencies.

halohalospecial commented 5 years ago

Better tell in doc that it requires elm-test@0.19-beta10 or later (until it's published, it requires node-test-runner@master, though)

Thanks for reminding me to add that to the docs! I was able to install elm-test@0.19-beta10 via npm -g install elm-test@beta.

It seems it's not working without Always Compile Main? While the option disabled, it shows this...

I could not replicate this. Can you give me a sample project? I used elm-spa-example to test.

Completions in test files are working for "type":"application" projects. Though in "type":"package" projects, there is a subtle awkwardness...

I'll check this out.

Side note: Now the workarounds described in #131 (comment) are no longer required. elm-explorations/test can now just live in test-dependencies.

Yep, no need for that workaround.

Thanks a lot, @ymtszw !

ymtszw commented 5 years ago

It seems it's not working without Always Compile Main? While the option disabled, it shows this...

I could not replicate this. Can you give me a sample project? I used elm-spa-example to test.

I can see the same error on RoutingTests.elm in elm-spa-example.

This happens only for test files when Always Compile Main option turned off. Application/package source files can be compiled regardless of the option.

Again, my environment is:

Windows 10
Nodejs 8.12.0 (binary built for Windows x86_64, not the one for Windows Subsystem of Linux)
Yarn 1.10.1
elm-test path: %USER%\AppData\Local\Yarn\bin\elm-test
elm path: %USER%\AppData\Local\Yarn\bin\elm

I'll try debugging a bit locally.

ymtszw commented 5 years ago

Using the same elm-test binary from Powershell, I might have found the cause.

Here, I'm purposefully inserting a typo in RoutingTests.elm for visibility, and partially mimicking arguments passed to elm-test in elm-make-runner.js of elmjutsu.

PS C:\Users\ymtszw\workspace\elm-spa-example> elm-test make --report=json C:\Users\ymtszw\workspace\elm-spa-example\tests\RoutingTests.elm
{"type":"error","path":null,"title":"NO INPUT","message":["What should I make though? I need more information, like:\n\n    ",{"bold":false,"underline":false,"color":"GREEN","string":"elm make src/Main.elm"},"\n    ",{"bold":false,"underline":false,"color":"GREEN","string":"elm make src/This.elm src/That.elm"},"\n\nHowever many files you give, I will create one JS file out of them."]}

PS C:\Users\ymtszw\workspace\elm-spa-example> elm-test make --report=json .\tests\RoutingTests.elm
{"type":"error","path":null,"title":"NO INPUT","message":["What should I make though? I need more information, like:\n\n    ",{"bold":false,"underline":false,"color":"GREEN","string":"elm make src/Main.elm"},"\n    ",{"bold":false,"underline":false,"color":"GREEN","string":"elm make src/This.elm src/That.elm"},"\n\nHowever many files you give, I will create one JS file out of them."]}

PS C:\Users\ymtszw\workspace\elm-spa-example> elm-test make --report=json tests\RoutingTests.elm
{"type":"compile-errors","errors":[{"path":"C:\\Users\\ymtszw\\workspace\\elm-spa-example\\tests\\RoutingTests.elm","name":"RoutingTests","problems":[{"title":"NAMING ERROR","region":{"start":{"line":17,"column":11},"end":{"line":17,"column":14}},"message":["I cannot find a `Tes` type:\n\n17| fromUrl : Tes\n              ",{"bold":false,"underline":false,"color":"red","string":"^^^"},"\nThese names seem close though:\n\n    ",{"bold":false,"underline":false,"color":"yellow","string":"Test"},"\n    ",{"bold":false,"underline":false,"color":"yellow","string":"Cmd"},"\n    ",{"bold":false,"underline":false,"color":"yellow","string":"Int"},"\n    ",{"bold":false,"underline":false,"color":"yellow","string":"List"},"\n\n",{"bold":false,"underline":true,"color":null,"string":"Hint"},": Read <https://elm-lang.org/0.19.0/imports> to see how `import`\ndeclarations work in Elm."]}]}]}

So basically, some patterns of Windows-style paths are not handled well in node-test-runner? I'm not sure this can be resolved from the caller (= elmjutsu) side, might be better escalating to node-test-runner.

Let me ping: @rtfeldman

halohalospecial commented 5 years ago

Interesting! Do you get the same "path" when using elm make?

ymtszw commented 5 years ago

Weird, no, "path" returned from elm make is not consistent with the one from elm-test make:

(Compile errors about UNKNOWN IMPORT are not a problem here since it's passing test files to elm make, obviously it should fail. At least it just works against any WIndows-style paths, absolute or relative)

PS C:\Users\ymtszw\workspace\elm-spa-example> elm make --report=json C:\Users\ymtszw\workspace\elm-spa-example\tests\RoutingTests.elm
{"type":"error","path":"C:\\Users\\ymtszw\\workspace\\elm-spa-example\\tests\\RoutingTests.elm","title":"UNKNOWN IMPORT","message":["The RoutingTests module has a bad import:\n\n    ",{"bold":false,"underline":false,"color":"RED","string":"import Test"},"\n\nI cannot find that module! Is there a typo in the module name?\n\nThe \"source-directories\" field of your elm.json tells me to only look in the src\ndirectory, but it is not there. Maybe it is in a package that is not installed\nyet?"]}

PS C:\Users\ymtszw\workspace\elm-spa-example> elm make --report=json .\tests\RoutingTests.elm
{"type":"error","path":".\\tests\\RoutingTests.elm","title":"UNKNOWN IMPORT","message":["The RoutingTests module has a bad import:\n\n    ",{"bold":false,"underline":false,"color":"RED","string":"import Test"},"\n\nI cannot find that module! Is there a typo in the module name?\n\nThe \"source-directories\" field of your elm.json tells me to only look in the src\ndirectory, but it is not there. Maybe it is in a package that is not installed\nyet?"]}

PS C:\Users\ymtszw\workspace\elm-spa-example> elm make --report=json tests\RoutingTests.elm
{"type":"error","path":"tests\\RoutingTests.elm","title":"UNKNOWN IMPORT","message":["The RoutingTests module has a bad import:\n\n    ",{"bold":false,"underline":false,"color":"RED","string":"import Test"},"\n\nI cannot find that module! Is there a typo in the module name?\n\nThe \"source-directories\" field of your elm.json tells me to only look in the src\ndirectory, but it is not there. Maybe it is in a package that is not installed\nyet?"]}

halohalospecial commented 5 years ago

Hi @ymtszw, were you able to resolve this? :)

ymtszw commented 5 years ago

Haven't checked for a while. I recently coding without Always Compile Main (my current project is getting bigger and full compile getting slower for on-save) so I'll look into it.

ymtszw commented 5 years ago

I think this is a path-handling issue of node-test-runner in Windows as mentioned before. Errors presented in https://github.com/halohalospecial/atom-elmjutsu/issues/131#issuecomment-429501564 is still there in elm-test@0.19.0-beta11. Posted here (https://github.com/rtfeldman/node-test-runner/issues/316)

As a workaround, pass project-relative file paths should work in Windows:

@@ -80,7 +80,13 @@ export default class ElmMakeRunner {
         }
       }
     } else {
-      filePathsToCompile = [editorFilePath];
+      if (isTestFilePath && process.platform === 'win32') {
+        // Workaround for https://github.com/halohalospecial/atom-elmjutsu/issues/131#issuecomment-429445645
+        const relativeEditorFilePath = path.relative(projectDirectory, editorFilePath);
+        filePathsToCompile = [relativeEditorFilePath];
+      } else {
+        filePathsToCompile = [editorFilePath];
+      }
     }

     const outputPath = '/dev/null'; // TODO: Make this configurable.

Tested on my local WIndows machine by editing the file under %UESR%\.atom\packages\. It passes file paths in a manner like tests\Examples.elm in Windows, which currently is the only pattern that works (elm-test@0.19.0-beta10,beta11).

halohalospecial commented 5 years ago

@ymtszw, do you want to submit a PR for your workaround?

ymtszw commented 5 years ago

@halohalospecial Happy to! #145 I'm on Mac machine right now but the patch is working on my Windows machine at home.

halohalospecial commented 5 years ago

Thanks a lot, @ymtszw ! :tada:

ymtszw commented 5 years ago

I believe this issue can be closed for now that all the good things are in and problems are solved. Thanks for the work @halohalospecial !

halohalospecial commented 5 years ago

Closing for now... Thanks, @ymtszw!

halohalospecial / atom-elmjutsu

Feature request: integrate elm-test #131