almightycouch / rethinkdb_ecto

RethinkDB adapter for Ecto.
https://hexdocs.pm/rethinkdb_ecto
MIT License
114 stars 18 forks source link

mix test is unable to run #27

Closed jfayad closed 8 years ago

jfayad commented 8 years ago

When I try to run mix test on an app using this package I get the following error:

** (CaseClauseError) no case clause matching: %RethinkDB.Exception.ConnectionClosed{} lib/rethinkdb_ecto.ex:185: RethinkDB.Ecto.storage_up/1 lib/mix/tasks/ecto.create.ex:36: anonymous fn/3 in Mix.Tasks.Ecto.Create.run/1 (elixir) lib/enum.ex:651: Enum."-each/2-lists^foreach/1-0-"/2 (elixir) lib/enum.ex:651: Enum.each/2 (mix) lib/mix/task.ex:296: Mix.Task.run_task/3 (mix) lib/mix/task.ex:328: Mix.Task.run_alias/3 (mix) lib/mix/task.ex:261: Mix.Task.run/2 (mix) lib/mix/cli.ex:58: Mix.CLI.run_task/2

redrabbit commented 8 years ago

This happens because you are not able to connect to the RethinkDB database.

jfayad commented 8 years ago

hmmm this possibility didn't cross my mind because mix ecto.migrate works successfully.

Why would mix test fail to connect and mix ecto.migrate (or ecto.rollback) succeed ?

cohawk commented 8 years ago

^^ Possibly because you do not have your database configured for the RethinkDB.Ecto adapter in config/test.exs ?

jfayad commented 8 years ago

Gush, you were right @cohawk thanks for pinpointing that.

Yet now I'm getting this error when trying to run the test:

** (UndefinedFunctionError) function Smartcat.Repo.__pool__/0 is undefined or private. Did you mean one of:

      * __log__/1

    (smartcat) Smartcat.Repo.__pool__()
    (ecto) Ecto.Adapters.SQL.Sandbox.mode/2
    (elixir) lib/code.ex:363: Code.require_file/2
    (elixir) lib/enum.ex:651: Enum."-each/2-lists^foreach/1-0-"/2
    (elixir) lib/enum.ex:651: Enum.each/2
cohawk commented 8 years ago

Does not appear that whichever test is running would be valid for RethinkDB.Ecto since the test appears to be using Ecto.Adapters.SQL.Sandbox.mode/2 which I believe it attempts to run DBConnection.Pool which RethinkDB.Ecto no longer uses?

jfayad commented 8 years ago

hmm and is there any way I could know which test is actually crashing the test suite ?

redrabbit commented 8 years ago

@cohawk, you are right, RethinkDB.Ecto does not support pooling. It uses a single (multiplexed) connection. Using the Ecto.Adapters.SQL.Sandbox will not work.

@jfayad, if you are using Phoenix and write tests using the generated ModelCase template, you will have to modify the template (test/support/model_case.ex) to skip the sandbox stuff.

As RethinkDB does not support running in a sandbox (no transaction/rollback), you might have to manually cleanup your database after each test. For example:

defmodule MyAppEctoTest do
  use ExUnit.case

  alias RethinkDB.Query as ReQL

  setup do
    # this function is called before each test,
    # you should cleanup (empty) your tables here.
    "my_table"
    |> ReQL.table()
    |> ReQL.delete()
    |> MyRepo.run()
    :ok
  end
end

You might also want to create a ExUnit.CaseTemplate for this purpose.

jfayad commented 8 years ago

Thanks @redrabbit for the details, I removed references to Sandbox in the model_cases being used, yet now I'm facing other errors when running basic tests generated by the phoenix generator.

Here are two types of failures I'm facing:

4) test renders page not found when id is nonexistent (Smartcat.OutdoorActionControllerTest)
     test/controllers/outdoor_action_controller_test.exs:35
     expected response status to be 404, but got 400 from:

     ** (Ecto.Query.CastError) deps/ecto/lib/ecto/repo/queryable.ex:295: value `"-1"` in `where` cannot be cast to type Ecto.UUID in query:

     from o in Smartcat.OutdoorAction,
       where: o.id == ^"-1",
       select: o

     stacktrace:
       (elixir) lib/enum.ex:1623: Enum."-reduce/3-lists^foldl/2-0-"/3
       (elixir) lib/enum.ex:1247: Enum."-map_reduce/3-lists^mapfoldl/2-0-"/3
       (elixir) lib/enum.ex:1623: Enum."-reduce/3-lists^foldl/2-0-"/3
       (ecto) lib/ecto/repo/queryable.ex:119: Ecto.Repo.Queryable.execute/5
       (ecto) lib/ecto/repo/queryable.ex:40: Ecto.Repo.Queryable.all/4
       (ecto) lib/ecto/repo/queryable.ex:72: Ecto.Repo.Queryable.one!/4
       (smartcat) web/controllers/outdoor_action_controller.ex:30: Smartcat.OutdoorActionController.show/2
       (smartcat) web/controllers/outdoor_action_controller.ex:1: Smartcat.OutdoorActionController.action/2
       (smartcat) web/controllers/outdoor_action_controller.ex:1: Smartcat.OutdoorActionController.phoenix_controller_pipeline/2
       (smartcat) lib/smartcat/endpoint.ex:1: Smartcat.Endpoint.instrument/4
       (smartcat) lib/phoenix/router.ex:261: Smartcat.Router.dispatch/2

and this one which is quite recurrent:

5) test does not update chosen resource and renders errors when data is invalid (Smartcat.OutdoorActionControllerTest)
     test/controllers/outdoor_action_controller_test.exs:54
     ** (Protocol.UndefinedError) protocol Enumerable not implemented for nil
     stacktrace:
       (elixir) lib/enum.ex:1: Enumerable.impl_for!/1
       (elixir) lib/enum.ex:116: Enumerable.reduce/3
       (elixir) lib/enum.ex:1636: Enum.reduce/3
       (elixir) lib/enum.ex:1623: Enum."-reduce/3-lists^foldl/2-0-"/3
       (rethinkdb_ecto) lib/rethinkdb_ecto.ex:299: RethinkDB.Ecto.execute_query/4
       (ecto) lib/ecto/repo/schema.ex:397: Ecto.Repo.Schema.apply/4
       (ecto) lib/ecto/repo/schema.ex:193: anonymous fn/11 in Ecto.Repo.Schema.do_insert/4
       (ecto) lib/ecto/repo/schema.ex:124: Ecto.Repo.Schema.insert!/4
       test/controllers/outdoor_action_controller_test.exs:55: (test)

FYI on my migration I have:

create table(:outdooractions, primary_key: false) do
      add :id, :uuid, primary_key: true

and on my schema I have:

@primary_key {:id, Ecto.UUID, autogenerate: true}

I tried to check some documentation on that topic specific to ecto.rethinkdb but couldn't find any. With my current knowledge I can't see what is wrong and I'm wondering if it's another unsupported feature ?

Thanks for the help

redrabbit commented 8 years ago

Hi @jfayad, the RethinkDB adapter is in early development stage, it is not a 1:1 replacement for SQL. Even if the adapter provides similar functionalities, you will have to modify some of your code to work with RethinkDB. It does not provide support for incremental integer primary keys nor constraints or unique indexes, transaction/rollback, pooling, etc.

In your schema/migration, the primary key cannot be set to Ecto.UUID, use :binary_id instead.

You might want to check out the Known Limitation from the docs.

jfayad commented 8 years ago

Indeed that was it, bad declaration of primary key.

I guess the foreign key is only there for when one is used ?

Also note that adding a "data reset" code is necessary for all tests to pass. If not then the following

test "updates chosen resource and redirects when data is valid", %{conn: conn} do
    outdoor_action = Repo.insert! %OutdoorAction{}
    conn = put conn, outdoor_action_path(conn, :update, outdoor_action), outdoor_action: @valid_attrs
    assert redirected_to(conn) == outdoor_action_path(conn, :show, outdoor_action)
    assert Repo.get_by(OutdoorAction, @valid_attrs)
  end

will fail at second run because the get_by is returning all results. This also mean that this test can't be called after another test which is inserting a document with @valid_attrs.. so order of the test will be important here... (I guess this is specific to RethinkDB since other DB will be able to leverage transactions and thus erase data right after it's been inserted, correct ?)

I also have a quick question (or more of a confirmation).

The following

test "shows chosen resource", %{conn: conn} do
    outdoor_action = Repo.insert! %OutdoorAction{}
    conn = get conn, outdoor_action_path(conn, :show, outdoor_action)
    assert html_response(conn, 200) =~ "Show outdoor action"
  end

Is inserting a document with no name eventhough the name has been set as required in a changeset. My understanding is that this code is shortcircuiting the changeset and directly inserts an emtpy Outdoor Action model object. Is that correct ? If that's the case one might be wondering why would the automatically generated test behave like that as this might confuse/mislead a newbie (like me :-) )

Thanks

redrabbit commented 8 years ago

Also note that adding a "data reset" code is necessary for all tests to pass.

Yes, that is what you can achieve with the setup macro (see my last comment). Also note that ExUnit does run your tests in a random order. You should not rely on the order anyway.

I guess this is specific to RethinkDB since other DB will be able to leverage transactions and thus erase data right after it's been inserted, correct ?

Yes, not really specific to RethinkDB only, but to databases without transaction support...

My understanding is that this code is shortcircuiting the changeset.

Ecto does not require changesets to work with the database. It can handle a Ecto.Schema directly.

It does not even require a schema and can work using the table names and maps/keywords directly:

[%{id: id}] = MyApp.Repo.insert_all "posts", [[title: "hello"]], returning: [:id]

Checkout the Ecto's insert_all and schema-less queries blog post.

Depending on what ORM you're used to, this can feel odd in the beginning but having schemas and changesets decoupled is a great thing. You can create schemas/changesets which do not reflect the actual database tables, this makes Ecto really flexible.

If that's the case one might be wondering why would the automatically generated test behave like that as this might confuse/mislead a newbie.

The generated test is only a really basic template for testing your schemas. It is not really "automatic" and does not provide any meaningful logic for testing your schemas.

jfayad commented 8 years ago

Great! that clears out quite some things. Thanks for that @redrabbit

I know i'm far from being an expert but if I can help with #26 please let me know as I have a very high interest in getting this solved and I'm usually a fast learner so with the right guidance I could "maybe" get things going.

jfayad commented 8 years ago

I know this is looking more like a conversation than an issue but on Slack rethinkDB channel there's no much activity.

@redrabbit I could see that RethinkDB doesn't support secondary indexes uniqueness and that the only way to ensure uniqueness on the DB level is to set the field as a primary Key. Yet according to the rethinkdb_ecto known limitations it seems only binary_id on the id field can be used as a primary key...

I tried to set the primary_key on the migration file to my string field and same on the model file but this didn't change. a table("users").info() in ReQL is always showing the id as the primary_key

How to tackle this issue when what I would like to do is count on a constraint in a changeset to ensure uniqueness of an email field ?

Thanks.

PS: if there is another place where it's better to have such discussion like exchanges, let me know