victorolinasc / junit-formatter

A JUnit XML report exporter for Elixir's ExUnit
https://hexdocs.pm/junit_formatter/
Apache License 2.0
110 stars 37 forks source link

Handle postgres constraints errors. #16

Closed PierrePIRONIN closed 7 years ago

PierrePIRONIN commented 7 years ago

I've got the same issue as @KronicDeth in #13 and in my case it was an incorrect handle of Postgres error which provides a nil message but got a message in a sub-attribute.

Feel free to modify the customized message ;)

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.7%) to 84.615% when pulling ee8bb89f0e50eeff9fda1f8f85312829e47cdfce on PierrePIRONIN:master into 550948bc9a15bf95851c8ccbddc714827ce90c9e on victorolinasc:master.

cs-victor-nascimento commented 7 years ago

Is there an easy way to reproduce the issue? The proposed solution seems a bit strange to me. Your clause is only giving a more descriptive text on the event of a Postgre error. Without your patch it would only be inspected (which would produce a string and end up in the report).

PierrePIRONIN commented 7 years ago

The message key is present in the reason struct but its value is nil, so the pattern matching enters in branch %{message: message} -> message not in other -> inspect(other) and so return nil.

The key here is to return reason.postGres.message instead of reason.message. I made the message a little bit more readable but a more raw solution could be :

message =
      case reason do
        %{postgres: postgresError} -> postgresError.message
        %{message: message} -> message
        other -> inspect(other)
      end

You can reproduce this issue by cloning `git@github.com:manchaques/micose-backend.git` and switching to `junit-formatter-issue-16` branch. You need a PostgreSQL database server.
cs-victor-nascimento commented 7 years ago

Oooooo I see! Tricky one :) Perhaps a better solution then would be to pattern match on nil message first than on message present. This way we would be safe if other libs or frameworks opted to go this route.

What do you say?

cs-victor-nascimento commented 7 years ago

Also, thanks a lot for digging into this issue!

PierrePIRONIN commented 7 years ago

That sounds great. Something like this ?

message =
      case reason do
        %{message: nil} -> inspect(reason)
        %{message: message} -> message
        other -> inspect(other)
      end
coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.7%) to 84.615% when pulling ef6fdc7edb2a9ee77f80be0ab22a4e59038746b0 on PierrePIRONIN:master into 550948bc9a15bf95851c8ccbddc714827ce90c9e on victorolinasc:master.

cs-victor-nascimento commented 7 years ago

Sorry for the huge delay!

I will add this to the next version. This is only missing tests. Could you add one please? Not sure how to make this happen in a test though as I tried with raise ArgumentError, message: nil and still it worked.

PierrePIRONIN commented 7 years ago

Hi, I'll try to write some tests soon !

cs-victor-nascimento commented 7 years ago

With your tests this will probably be 1.2.1.

Thanks once again!

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.3%) to 86.538% when pulling 3dd8f770faface74766e35dae3f31440f27c4615 on PierrePIRONIN:master into 550948bc9a15bf95851c8ccbddc714827ce90c9e on victorolinasc:master.