Skip to content

Update error message to match on OTP 24 #47

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

Merged

Conversation

LostKobrakai
Copy link
Contributor

No description provided.

@@ -2370,9 +2370,20 @@ defmodule Ecto.Adapters.SQLite3.ConnectionTest do
end

test "create table with an unsupported type" do
msg =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think it is fine to just return the multi string error message regardless of OTP version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message afaik is not in our control. The difference is due to improvments on error messages for NIFs and BIFs, which are part of OTP24.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. I don't think inside the NIF we raise any exceptions there, just return atom error tuples. 👍🏻

@warmwaffles warmwaffles merged commit 8ff7c8d into elixir-sqlite:main Aug 25, 2021
@warmwaffles
Copy link
Member

@LostKobrakai I updated our workflow to include OTP24, 23, and 22 tests and we have two failures around this. I don't think it was introduced by you. Probably something left over in a test that we forgot about. #54

@LostKobrakai
Copy link
Contributor Author

It might be that the new errors not only need OTP24, but also some integration in elixir 1.12

@warmwaffles
Copy link
Member

Perhaps the better solution @LostKobrakai would be to remove the message for the assertion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants