DockYard / valid_field

https://hex.pm/packages/valid_field
MIT License
48 stars 6 forks source link

Test against elixir 1.2.6 and 1.3.2 on travis #16

Closed gmile closed 7 years ago

gmile commented 8 years ago

The build appears to be green on 1.2 (yet red on 1.3), so it's good to know the source code is not bound to 1.1.1.

bcardarella commented 8 years ago

:+1: thanks!

gmile commented 8 years ago

@bcardarella if you're open to merging this change, please don't merge it just yet. I'm checking if minor version of 1.2.x will pass as well. If so, will update the PR to include the latest in the series, e.g. 1.2.6.

bcardarella commented 8 years ago

Its strange to fail on 1.3 though. Might be an assertion matcher issue

bcardarella commented 8 years ago

@gmile the errors on 1.3 are the result of 1.3 ExUnit.Assertion adding some newlines into the resulting message. I can update this to be more flexible

gmile commented 8 years ago

@bcardarella noted, I was seeing the \n errors, but was unable to figure out what in newer version of elixir causes this. What was the thought process you took to figure out the fault was in changes made to ExUnit.Assertion? :)

I can update this to be more flexible

In that case I am updating the .travis.yml to include 1.3.1 as well. Would you like to keep 1.1.1 and 1.2.6 too? Or testing against the latest stable release is fine?

bcardarella commented 8 years ago

@gmile it is happening here: https://github.com/elixir-lang/elixir/blob/master/lib/ex_unit/lib/ex_unit/assertions.ex#L35-L41

My solution has been to create a custom assertion to use instead of ExUni.Assertion. I don't entirely understand the diffing yet so I've been working around it

gmile commented 8 years ago

Regarding test failures. I see two kinds of test failures here:

  1. doc tests,
  2. regular tests.

So if I understand correctly, tests for valid_field library work the following way: assert_raise is being made on a result (a message) of another assertion (assert, inside this function for instance).

When not met, assert inside a function returns a message that is formatted (via this or this, I'm not sure which one exactly).

I think it doesn't make sense to assert on formatted message, constructed by ExUnit.AssertionError/ExUnit.MultiError. A somewhat better, more controllable way to implement this would be raising a custom exception.

It seems like working around the diff can do the job for normal tests. But for doctests this will not work, unless there's a way to put a formatted ex_unit output to doctest.

Best way to solve this, imo, is to:

  1. introduce some form of custom exception, e.g. MyException,
  2. in methods from ValidField, use raise manually, e.g.:
if invalid_values != [] do
  raise MyException, "Expected the following values to be valid..."
end
gmile commented 8 years ago

Out of curiosity I've made the assert_valid_field function to raise an ArithmeticError. The doc test passed:

diff --git a/lib/valid_field.ex b/lib/valid_field.ex
index a451b0a..ebec31a 100644
--- a/lib/valid_field.ex
+++ b/lib/valid_field.ex
@@ -15,7 +15,7 @@ defmodule ValidField do
       ...> |> ValidField.assert_valid_field(:last_name, ["Value"])
       iex> ValidField.with_changeset(%Model{})
       ...> |> ValidField.assert_valid_field(:first_name, [nil, ""])
-      ** (ExUnit.AssertionError) Expected the following values to be valid for "first_name": nil, ""
+      ** (ArithmeticError) Expected the following values to be valid for "first_name": nil, ""
   """
   @spec assert_valid_field(map, atom, list) :: map
   def assert_valid_field(changeset, field, values) do
@@ -24,7 +24,9 @@ defmodule ValidField do
       |> map_value_assertions(field, values)
       |> Enum.filter_map(fn {_key, value} -> value end, fn {key, _value} -> key end)

-    assert invalid_values == [], "Expected the following values to be valid for #{inspect Atom.to_string(field)}: #{_format_values invalid_values}"
+    if invalid_values != [] do
+      raise ArithmeticError, "Expected the following values to be valid for #{inspect Atom.to_string(field)}: #{_format_values invalid_values}"
+    end

     changeset
   end
bcardarella commented 8 years ago

I think the path forward here is migrating away from ExUnit.Assertion and using a custom module for raising.

gmile commented 8 years ago

@bcardarella would you be interested in reviewing a PR addressing it? I can work on something

bcardarella commented 8 years ago

Yup, go for it :)

gmile commented 8 years ago

Force-pushed the branch to trigger Travis build. It's now green.

gmile commented 8 years ago

Bumped elixir to 1.3.2

bcardarella commented 7 years ago

We just bumped test support to 1.5.0 on travis.