jfpedroza / neotest-elixir

Neotest adapter for Elixir
MIT License
38 stars 10 forks source link

change position id to work for dynamic tests #3

Closed ajayvigneshk closed 1 year ago

ajayvigneshk commented 1 year ago
ajayvigneshk commented 1 year ago

Marking it draft since I'm not sure if I've covered all cases and would like some feedback if the approach is okay

jfpedroza commented 1 year ago

I like the approach. I was thinking about using the test lines as well differently, but this is simpler.

I'll try to test it on my own tonight.

jfpedroza commented 1 year ago

I'm trying this PR. The position id seems to work correctly.

I found an issue where the output of the command doesn't clear between runs. Like if you do NeotestOutput at the top of the file, you will see the output repeated and every time you run, it adds to that same output.

It doesn't happen when I switch back to master.

ajayvigneshk commented 1 year ago

I just rebased on master, and couldn't reproduce the issue. Could you try again to see if the issue still persists?

ajayvigneshk commented 1 year ago

I didn't notice the streaming results part. I think I should update code there as well?

jfpedroza commented 1 year ago

I just rebased on master, and couldn't reproduce the issue. Could you try again to see if the issue still persists?

Indeed, I couldn't reproduce it either. Seems to be gone.

jfpedroza commented 1 year ago

I didn't notice the streaming results part. I think I should update code there as well?

I think so, yes.

jfpedroza commented 1 year ago

I think this is ready!

ajayvigneshk commented 1 year ago

One scenario I noticed is when there's a test like this

    for i <- [1, 2, 3] do
      test "weird test: #{i}" do
        if unquote(i) == 2 do
          assert false
        else
          assert true
        end
      end
    end

that passes for 1,3 but fails for 2. The sign indicator shows a pass (green tick) first then changes to error(:red tick). Due to streaming the above mentioned transition (pass to error) happens twice. I think it would be hard to fix this right now, but we can consider it as an improvement to fine tune later on

jfpedroza commented 1 year ago

Yes, that can be left as an improvement.