Change percentiles type in Measurements to :map

[tallysmartins]

Its a bug on Ecto. elixir-ecto/ecto#2637 We can solve it temporaly by changing the field type to just :map.

[coveralls]

Coverage remained the same at 83.173% when pulling 6fd57e0 on tallysmartins:fix-percentiles-field into 424f013 on elixir-bench:master.

[PragTob]

Great job figuring it out and reporting the bug upstream to ecto btw. 🎉

[PragTob]

Looks good to me, minor thing though - can we write a test to reproduce the problem here?

a.) that gives us confidence
b.) we can change it back with a new ecto version and have an automated way of checking

[tallysmartins]

Added some tests that will fail when we update our Ecto with the bug fix.

I tried to use the setup_all and on_exit callbacks to reduce code duplication with the functions that Migrate and delete the tables I use in the tests but I got this errors about process ownership, do you know what can be?

 3) ElixirBench.RepoTest: failure on setup_all callback, test invalidated                                                                                                                    
     ** (DBConnection.OwnershipError) cannot find ownership process for #PID<0.364.0>.
     
     When using ownership, you must manage connections in one
     of the four ways:
     
     * By explicitly checking out a connection
     * By explicitly allowing a spawned process
     * By running the pool in shared mode
     * By using :caller option with allowed process
     
     The first two options require every new process to explicitly
     check a connection out or be allowed by calling checkout or
     allow respectively.
     
     The third option requires a {:shared, pid} mode to be set.
     If using shared mode in tests, make sure your tests are not
     async.
     
     The fourth option requires [caller: pid] to be used when
     checking out a connection from the pool. The caller process
     should already be allowed on a connection.
     
     If you are reading this error, it means you have not done one
     of the steps above or that the owner process has crashed.
     
     See Ecto.Adapters.SQL.Sandbox docs for more information.

[PragTob]

Hey @tallysmartins sorry I missed this one :)

I think I didn't put my description correctly - I didn't want to have a test that fails when we update ecto but rather a test that fails if we don't change this. But so that if we update ecto and we remove our workaround the test assures us that everything still works fine. So a reproduction of the scenario that lead to the bug so that we can make sure both our current workaround and any future solutions might work :)

[tallysmartins]

Oh, I got that... I will write a scenario to our Measurements that breaks if we don't change to :map.. Similarly to the ones I've added to Repo.

And now that I've already added those ones can we stick with them to get noticed when ecto gets fixed?

[PragTob]

I'm not sure having specs to get noticed when ecto change is a good thing to have :D I mean now that we have tem we can keep them but they wouldn't be worth the trouble from my point of view and might be error prone

[tallysmartins]

    # field :percentiles, {:map, :float} => this test fail
    # field :percentiles, :map => this test succeed
    test "not raise when exponential notation is given to percentiles" do
      percentiles = %{"50" => 4.165e5, "99" => 4.165e5}
      measurement = build(:measurement)

      changeset = Measurement.changeset(measurement, %{@attrs | "percentiles" => percentiles})
      assert {:ok, _} = Repo.insert(changeset)

      measurement = Repo.one(Measurement)
      assert %{"50" => 416_500, "99" => 416_500} = measurement.percentiles
    end

looks good to have just this one? and do I leave the comments in the begining?