Skip to content
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

Only run tests via commands #48

Open
razzeee opened this issue Jun 3, 2019 · 19 comments
Open

Only run tests via commands #48

razzeee opened this issue Jun 3, 2019 · 19 comments
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@razzeee
Copy link
Member

razzeee commented Jun 3, 2019

No description provided.

@andys8
Copy link
Member

andys8 commented Jun 3, 2019

Cross link discussion: #47 (comment)

@razzeee
Copy link
Member Author

razzeee commented Jun 7, 2019

The real question is how do we achieve this?

@razzeee razzeee added the enhancement New feature or request label Jun 14, 2019
@razzeee
Copy link
Member Author

razzeee commented Jul 14, 2019

We should probably add a commands for running all tests. At the moment we only run the tests from the file your in on save.

@razzeee razzeee added the help wanted Extra attention is needed label Jul 14, 2019
@andys8
Copy link
Member

andys8 commented Jul 14, 2019

We're back to "executing" tests? If so, I think there has to be an opt-out.

It takes 15 seconds for a toy project with fuzz tests. A well tested production project might be even worse.

There is and should be a big difference between making sure, the test is valid elm syntax, and execution the tests. I'm not sure, but it looks like both is done. Is this correct?

Can't really test because of issue file path. See slack :)

@andys8
Copy link
Member

andys8 commented Jul 14, 2019

This is the current result:

[Trace - 3:23:19 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///home/as/dev/repository/vim-emulation/tests/MainTest.elm",
    "diagnostics": [
        {
            "range": {
                "start": {
                    "line": 0,
                    "character": 0
                },
                "end": {
                    "line": 0,
                    "character": 0
                }
            },
            "message": "Equality - Comparison: Expect.equal\n Expected: Insert\n Actual: Normal\n",
            "severity": 1,
            "source": "Elm"
        }
    ]
}

There is currently no line number or test name. So it's hard to tell which test failed.

@razzeee
Copy link
Member Author

razzeee commented Jul 14, 2019

Those are not provided by elm test unfortunately.

Best would be to add commands to run these. Until the language server spec implement's something test related.

@andys8
Copy link
Member

andys8 commented Jul 14, 2019

It provides the test name as labels:

{
  "event": "testCompleted",
  "status": "fail",
  "labels": [
    "MainTest",
    "Main",
    "update",
    "Type a word"
  ],
  "failures": [
    {
      "given": null,
      "message": "Expect.equal",
      "reason": {
        "type": "Equality",
        "data": {
          "expected": "Buffer \"asd1f\"",
          "actual": "Buffer \"asdf\"",
          "comparison": "Expect.equal"
        }
      }
    }
  ],
  "duration": "5"
}

But I'm also very much in favor of using commands instead for now.

@andys8
Copy link
Member

andys8 commented Jul 14, 2019

Command would be:

  • All: elm-test in root project directory
  • Specific file: elm-test tests/MyTest.elm

@razzeee
Copy link
Member Author

razzeee commented Jul 14, 2019

so how would the labels need to be concated to form the name again?

@andys8
Copy link
Member

andys8 commented Jul 14, 2019

It looks like the labels are:

  • name of the test module
  • n describe blocks
  • name of the test function

@andys8
Copy link
Member

andys8 commented Jul 14, 2019

Intersperse the list with > should probably look fine.

@razzeee
Copy link
Member Author

razzeee commented Jul 14, 2019

Added

@andys8
Copy link
Member

andys8 commented Jul 14, 2019

Nice. Do you plan to change it to a command instead of on save?

@razzeee
Copy link
Member Author

razzeee commented Jul 14, 2019

Probably, but commands are at the moment, what I keep pushing back.

@andys8
Copy link
Member

andys8 commented Jul 14, 2019

Yeah, I try to push back executing tests on save, because I won't be able to upgrade then :D

Edit: Configuration is another option, of course.

@razzeee
Copy link
Member Author

razzeee commented Jul 14, 2019

Which is confusing to me, as this was already included in the last release

@andys8
Copy link
Member

andys8 commented Jul 14, 2019

I didn't notice because I didn't write new tests in the last weeks ;) At one point I split up a large test file, because it became unusable.

Here is a demo with latest release. It shows, how long it takes to remove/fix a syntax error, because this'll trigger executing the test. Around 20 seconds. The file is already smaller, because I split up the tests, because of lagging at one point.

asciicast

@razzeee razzeee changed the title Elm test should be a first class citizen Only run tests via commands Jul 15, 2019
@razzeee
Copy link
Member Author

razzeee commented Jul 15, 2019

I pushed this WIP branch. https://github.com/elm-tooling/elm-language-server/tree/test-commands

It has two problems:

  1. Running all tests -> test runner doesn't say which file it's respond is for testCompleted event should contain file path rtfeldman/node-test-runner#371
  2. Running tests from the current file -> Commands doesn't seem to be able to figure out the current file. Might have overlooked something.

So I would basically rip out test support for the moment.

@andys8
Copy link
Member

andys8 commented Jul 16, 2019

Hey, didn't have a look at the branch, but regarding 1: Is the assumption holding that the first label is always the module name and can therefore deterministicly converted to file name? Is this the case or are there counter examples?

Update: rtfeldman/node-test-runner#371 (comment)

@razzeee razzeee added this to the Next milestone Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants