Update underscore/1 to support Elixir 1.6's regex #275

unnawut · 2018-03-06T11:11:55Z

Regex.split/3's behavior was changed in Elixir 1.6 and caused ExMachina.Strategy.underscore/1 to return an extra underscore and broke all factory names that are generated by ExMachina.Strategy.name_from_struct/1.

This PR does ~~2 things~~ 1 thing:

~~Fixes the regex inside ExMachina.Strategy.underscore/1 to do negative lookbehind for a start of line.~~
~~Use a single Regex.replace/4 instead of Regex.split/3 + Enum.join/2 which, according to Jose Valim, is equivalent.~~
Use Macro.underscore/1 instead of Regex.split/3 + Enum.join/2 to convert module name to snake case. (Thanks @vernomcrp for the suggestion!)

Before

Elixir 1.5.3 (success)

$ elixir --version
Erlang/OTP 20 [erts-9.2.1] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:10] [hipe] [kernel-poll:false]

Elixir 1.5.3

$ mix test
The database for ExMachina.TestRepo has been dropped
The database for ExMachina.TestRepo has been created
................................................................

Finished in 0.3 seconds
64 tests, 0 failures

Randomized with seed 941966

Elixir 1.6.2 (failed)

$ elixir --version
Erlang/OTP 20 [erts-9.2.1] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:10] [hipe] [kernel-poll:false]

Elixir 1.6.2 (compiled with OTP 20)

$ mix test
The database for ExMachina.TestRepo has been dropped
The database for ExMachina.TestRepo has been created


  1) test name_from_struct returns the factory name based on passed in struct (ExMachina.StrategyTest)
     test/ex_machina/strategy_test.exs:23
     Assertion with == failed
     code:  assert ExMachina.Strategy.name_from_struct(%{__struct__: User}) == :user
     left:  :_user
     right: :user
     stacktrace:
       test/ex_machina/strategy_test.exs:24: (test)

...............................................................

Finished in 0.3 seconds
64 tests, 1 failure

Randomized with seed 568658

After

Elixir 1.5.3 (success)

$ elixir --version
Erlang/OTP 20 [erts-9.2.1] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:10] [hipe] [kernel-poll:false]

Elixir 1.5.3

$ mix test
The database for ExMachina.TestRepo has been dropped
The database for ExMachina.TestRepo has been created
................................................................

Finished in 0.3 seconds
64 tests, 0 failures

Randomized with seed 801432

Elixir 1.6.2 (success)

$ elixir --version
Erlang/OTP 20 [erts-9.2.1] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:10] [hipe] [kernel-poll:false]

Elixir 1.6.2 (compiled with OTP 20)

$ mix test
Compiling 3 files (.ex)
The database for ExMachina.TestRepo has been dropped
The database for ExMachina.TestRepo has been created
................................................................

Finished in 0.5 seconds
64 tests, 0 failures

Randomized with seed 824888

pdawczak · 2018-03-06T11:18:15Z

Would this change remain backward compatible, so if someone has project with Elixir <1.6 this still works?

unnawut · 2018-03-06T11:23:04Z

I ran mix test on Elixir 1.5.3 and 1.6.2 with 64 tests passing. Before this PR it would return "64 tests, 1 failure" on 1.6.2

Not sure how many versions back I need to test with?

unnawut · 2018-03-06T11:27:02Z

And these are the existing tests that should cover this change:

/test/ex_machina/strategy_test.exs:

  test "name_from_struct returns the factory name based on passed in struct" do
    assert ExMachina.Strategy.name_from_struct(%{__struct__: User}) == :user
    assert ExMachina.Strategy.name_from_struct(%{__struct__: TwoWord}) == :two_word
    assert ExMachina.Strategy.name_from_struct(%{__struct__: NameSpace.TwoWord}) == :two_word
  end

pdawczak · 2018-03-06T11:45:00Z

Thanks @unnawut for your answer. I don't think there is need to go back any further. From my point of view this one failing example between versions is indicative enough.

unnawut · 2018-03-06T12:53:54Z

I was just suggested a better solution with Macro.underscore/1. I'll update it so please refrain from merging.

unnawut · 2018-03-06T16:40:32Z

@pdawczak I've made the following changes to the PR:

Use Macro.underscore/1 instead of the custom ExMachina.Strategy.underscore/1 implementation.
Added test results to the PR's description.

Feel free to review and merge! :)

unnawut · 2018-03-08T04:29:43Z

@paulcsmith @jsteiner @germsvel Since this pretty much breaks ex_machina on Elixir 1.6, would you mind have a look at this PR, and perhaps make a new release for 1.6?

germsvel · 2018-03-10T14:13:12Z

Thanks for catching this @unnawut! Macro.underscore was introduced in 1.4 at some point, right? I wonder if anybody is still using elixir 1.3.x? I really would like to use Macro.underscore since it's much cleaner, but I wonder if it would be safer to just use Regex.replace/4 like you had done originally?

unnawut · 2018-03-10T14:50:25Z

Actually Macro.underscore/1 was introduced since 1.2 in 2015 as confirmed by the release notes and the docs.

Apart from code formatting and adding an extra pattern matching for empty strings in the blame, nothing else has changed. So I think it's safe enough...

unnawut · 2018-03-10T16:04:49Z

@germsvel I have (two) good news and (one) bad news....

Good news # 1: Just to be sure here, I tested this PR against 1.4.5. It's working fine on 1.4.5.

$ elixir --version
Erlang/OTP 20 [erts-9.2.1] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:10] [hipe] [kernel-poll:false]

Elixir 1.4.5

$ mix test
The database for ExMachina.TestRepo has been dropped
The database for ExMachina.TestRepo has been created
................................................................

Finished in 0.3 seconds
64 tests, 0 failures

Randomized with seed 544101

Good news # 2: The ExMachina.StrategyTest which is affected by this PR is passing on 1.3.4.

$ elixir --version
Erlang/OTP 19 [erts-8.3.5.4] [source] [64-bit] [smp:4:4] [async-threads:10] [hipe] [kernel-poll:false]

Elixir 1.3.4

$ mix test test/ex_machina/strategy_test.exs
The database for ExMachina.TestRepo has been dropped
The database for ExMachina.TestRepo has been created
..

Finished in 0.1 seconds
2 tests, 0 failures

Randomized with seed 468451

Bad news: Tests failed in other areas unrelated to this PR on 1.3.4:

$ elixir --version
Erlang/OTP 19 [erts-8.3.5.4] [source] [64-bit] [smp:4:4] [async-threads:10] [hipe] [kernel-poll:false]

Elixir 1.3.4

$ mix test
The database for ExMachina.TestRepo has been dropped
The database for ExMachina.TestRepo has been created
...........................................................
23:00:56.003 [error] GenServer ExMachina.Sequence terminating
** (UndefinedFunctionError) function List.pop_at/2 is undefined or private
    (elixir) List.pop_at(["A", "B", "C"], 0)
    (ex_machina) lib/ex_machina/sequence.ex:68: anonymous fn/4 in ExMachina.Sequence.next/2
    (elixir) lib/agent/server.ex:16: Agent.Server.handle_call/3
    (stdlib) gen_server.erl:615: :gen_server.try_handle_call/4
    (stdlib) gen_server.erl:647: :gen_server.handle_msg/5
    (stdlib) proc_lib.erl:247: :proc_lib.init_p_do_apply/3


  1) test traverses a list each time it is called (ExMachina.SequenceTest)
     test/ex_machina/sequence_test.exs:15
     ** (exit) exited in: GenServer.call(ExMachina.Sequence, {:get_and_update, #Function<1.78584464/1 in ExMachina.Sequence.next/2>}, 5000)
         ** (EXIT) an exception was raised:
             ** (UndefinedFunctionError) function List.pop_at/2 is undefined or private
                 (elixir) List.pop_at(["A", "B", "C"], 0)
                 (ex_machina) lib/ex_machina/sequence.ex:68: anonymous fn/4 in ExMachina.Sequence.next/2
                 (elixir) lib/agent/server.ex:16: Agent.Server.handle_call/3
                 (stdlib) gen_server.erl:615: :gen_server.try_handle_call/4
                 (stdlib) gen_server.erl:647: :gen_server.handle_msg/5
                 (stdlib) proc_lib.erl:247: :proc_lib.init_p_do_apply/3
     stacktrace:
       (elixir) lib/gen_server.ex:604: GenServer.call/3
       test/ex_machina/sequence_test.exs:16: (test)



  2) test updates different sequences independently (ExMachina.SequenceTest)
     test/ex_machina/sequence_test.exs:22
     ** (exit) exited in: GenServer.call(ExMachina.Sequence, {:update, #Function<3.78584464/1 in ExMachina.Sequence.reset/0>}, 5000)
         ** (EXIT) no process
     stacktrace:
       (elixir) lib/gen_server.ex:596: GenServer.call/3
       test/ex_machina/sequence_test.exs:1: ExMachina.SequenceTest.__ex_unit__/2



  3) test increments the sequence each time it is called (ExMachina.SequenceTest)
     test/ex_machina/sequence_test.exs:10
     ** (exit) exited in: GenServer.call(ExMachina.Sequence, {:update, #Function<3.78584464/1 in ExMachina.Sequence.reset/0>}, 5000)
         ** (EXIT) no process
     stacktrace:
       (elixir) lib/gen_server.ex:596: GenServer.call/3
       test/ex_machina/sequence_test.exs:1: ExMachina.SequenceTest.__ex_unit__/2



  4) test can reset sequences (ExMachina.SequenceTest)
     test/ex_machina/sequence_test.exs:40
     ** (exit) exited in: GenServer.call(ExMachina.Sequence, {:update, #Function<3.78584464/1 in ExMachina.Sequence.reset/0>}, 5000)
         ** (EXIT) no process
     stacktrace:
       (elixir) lib/gen_server.ex:596: GenServer.call/3
       test/ex_machina/sequence_test.exs:1: ExMachina.SequenceTest.__ex_unit__/2



  5) test let's you quickly create sequences (ExMachina.SequenceTest)
     test/ex_machina/sequence_test.exs:29
     ** (exit) exited in: GenServer.call(ExMachina.Sequence, {:update, #Function<3.78584464/1 in ExMachina.Sequence.reset/0>}, 5000)
         ** (EXIT) no process
     stacktrace:
       (elixir) lib/gen_server.ex:596: GenServer.call/3
       test/ex_machina/sequence_test.exs:1: ExMachina.SequenceTest.__ex_unit__/2



Finished in 0.3 seconds
64 tests, 5 failures

Randomized with seed 594070

... in which List.pop_at/2 does not exist in 1.3.4.

My suggestion is to drop support for 1.3.* since these test failures should have been around for 5 months already and no one reported it yet. So I think this PR is safe enough.

germsvel · 2018-03-10T17:03:17Z

Awesome. Thanks for all the research @unnawut! That was super helpful. I think this is definitely good then.

Bump version to 2.2.0. Notable changes include, - Support for lists in sequences (PR #227) - Using `Macro.underscore/1` so that factory names don't break in elixir 1.6 (PR #275)

germsvel · 2018-03-13T23:13:25Z

@unnawut I released version 2.2.0 of ex_machina today. Please open an issue if you find any problems, and thanks again for contributing!

unnawut · 2018-03-14T03:00:31Z

Thank you!

Update underscore/1 to support Elixir 1.6's regex

56260a3

Use Macro.underscore/1 instead of a custom implementation

f3d75d4

germsvel merged commit 037d826 into beam-community:master Mar 10, 2018

unnawut deleted the dev-underscore-regex branch March 10, 2018 18:42

unnawut mentioned this pull request Mar 12, 2018

Upgrade to Elixir 1.6 omgnetwork/ewallet#109

Merged

germsvel added a commit that referenced this pull request Mar 13, 2018

Bump version to 2.2.0

c06f212

Bump version to 2.2.0. Notable changes include, - Support for lists in sequences (PR #227) - Using `Macro.underscore/1` so that factory names don't break in elixir 1.6 (PR #275)

germsvel mentioned this pull request Mar 13, 2018

Bump version to 2.2.0 #276

Merged

germsvel added a commit that referenced this pull request Mar 13, 2018

Bump version to 2.2.0 (#276)

466ad05

Bump version to 2.2.0. Notable changes include, - Support for lists in sequences (PR #227) - Using `Macro.underscore/1` so that factory names don't break in elixir 1.6 (PR #275)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update underscore/1 to support Elixir 1.6's regex #275

Update underscore/1 to support Elixir 1.6's regex #275

unnawut commented Mar 6, 2018 •

edited

Loading

pdawczak commented Mar 6, 2018

unnawut commented Mar 6, 2018 •

edited

Loading

unnawut commented Mar 6, 2018

pdawczak commented Mar 6, 2018

unnawut commented Mar 6, 2018

unnawut commented Mar 6, 2018

unnawut commented Mar 8, 2018

germsvel commented Mar 10, 2018

unnawut commented Mar 10, 2018

unnawut commented Mar 10, 2018 •

edited

Loading

germsvel commented Mar 10, 2018

germsvel commented Mar 13, 2018

unnawut commented Mar 14, 2018

Update underscore/1 to support Elixir 1.6's regex #275

Update underscore/1 to support Elixir 1.6's regex #275

Conversation

unnawut commented Mar 6, 2018 • edited Loading

Before

Elixir 1.5.3 (success)

Elixir 1.6.2 (failed)

After

Elixir 1.5.3 (success)

Elixir 1.6.2 (success)

pdawczak commented Mar 6, 2018

unnawut commented Mar 6, 2018 • edited Loading

unnawut commented Mar 6, 2018

pdawczak commented Mar 6, 2018

unnawut commented Mar 6, 2018

unnawut commented Mar 6, 2018

unnawut commented Mar 8, 2018

germsvel commented Mar 10, 2018

unnawut commented Mar 10, 2018

unnawut commented Mar 10, 2018 • edited Loading

germsvel commented Mar 10, 2018

germsvel commented Mar 13, 2018

unnawut commented Mar 14, 2018

unnawut commented Mar 6, 2018 •

edited

Loading

unnawut commented Mar 6, 2018 •

edited

Loading

unnawut commented Mar 10, 2018 •

edited

Loading