0

The following property test is working fine, however, I think there should be a better and more efficient way of implementing this.

params in the following property will be something like this:

%{
  "project_id" => "%&!XX!hLCfsS-dO_<fy?kpi4y=AEumQ$Xn:#.7Fl TnH~k>ZLB[q",
  "task_id" => [
    %{"asset_id" => 10, "tasks" => []},
    %{"asset_id" => 10, "tasks" => []}
  ]
}

Property Testing:

property "bad project value" do
   [user, project] = prepare()
   user_gen = constant(%{id: user.id})

   project_gen =  constant("project_id")
                  |> map_of(Factory.my_terms, length: 1)

   tasks = constant(%{"asset_id" => 10, "tasks" => []})
          |> list_of(length: 2)
   tasks_gen = constant("task_id")
               |> map_of(tasks, length: 1)

   check all project <- project_gen, task <- tasks_gen , user <- user_gen do
     params = Map.merge(project, task)
     res = ProjectTask.Save.save(params, user)
     assert res == {:error, :not_found}
   end

Factory.my_terms is the following:

def my_terms() do
  one_of([string(:alphanumeric), string(:ascii), atom(:alphanumeric), integer(), binary()])
end

UPDATE

  property "bad project value" do
      [user, project] = prepare()
      project_gen =  constant("project_id")
                     |> map_of(Factory.my_terms, length: 1)
      tasks = List.duplicate(%{"asset_id" => 10, "tasks" => []}, 2)
      tasks = %{"tasks" => tasks}
      check all project <- project_gen do
        params = Map.merge(project, tasks)
        res = ProjectTask.Save.save(params, %{id: user.id})
        assert res == {:error, :not_found}
      end
    end
Mr H
  • 5,254
  • 3
  • 38
  • 43
  • What exactly do you want to test? Also, while it uses a ton of `StreamData` syntax, it’s hardly a property test. You change `project_id` only and the very same effect might be easily achieved without `StreamData` at all, just by doing the normal loop with random `my_terms` (generating random binary is easy without any `StreamData`.) Also, I doubt your `my_terms` generates what you actually wanted (e. g. it might generate `:foo` or `42`.) – Aleksei Matiushkin Mar 20 '19 at 05:34
  • For instance, `tasks = constant(%{"asset_id" => 10, "tasks" => []}) |> list_of(length: 2)` is by no mean better (although way more cryptic) than `tasks = List.duplicate(%{"asset_id" => 10, "tasks" => []}, 2)`. – Aleksei Matiushkin Mar 20 '19 at 05:37
  • You are 100% correct. I have put the update in there now, which is cleaner. I have kept the `my_terms` I don't mind to make sure things like `:foo` or `nil` or some `binary` can break anything in my code. – Mr H Mar 20 '19 at 06:03
  • The main question remains: **what are you indeed testing?** It’s unclear from the code you posted. You save the task and check that `ProjectTask.Save.save/1` returns an error. Why would `save` return an error? What are the circumstances? Usually, your test should check that good input results in a success and bad input results in an error. What are success and failure in this case? – Aleksei Matiushkin Mar 20 '19 at 06:08
  • Fair point. this function: `Storage.ProjectTask.Save.save` will save the a list of tasks for a project. When the project data structure goes into this module, I check to make sure if user allowed to, then I save the tasks against the requested project. I have a unit test that I push a project with a valid `id:integer` and valid asset id and task id. Then I expect a return of a correct data structure. very similar to what went in. So with Property listing I need make sure that if invalid data type could get in, it wont crash it. – Mr H Mar 20 '19 at 06:24
  • I have more property testing where I am testing the `tasks` with different data type. Things like, if `:foo` got in, or if `id: 0` or `id: nil` or some funny text and stuff like that – Mr H Mar 20 '19 at 06:30
  • What would be wrong with a plain old good unit test for the failure as well? You validate `user.id`, the shape of the `project_id` has nothing to do with it. Property testing is great when the input is undetermined, but in your case it’s perfectly determined. I see you wanted to use fashionable tech :) but in this case it makes zero sense. – Aleksei Matiushkin Mar 20 '19 at 06:30
  • If your methods allow passing `id: 0` or `id: nil`, you have a serious design flaw. Instead of testing weird input, you should restrict the input with guards and pattern matching. Also, use `@spec` everywhere and do a static analysis with `dialyzer`. – Aleksei Matiushkin Mar 20 '19 at 06:32
  • I am already doing what you are suggested, and I have document for every module that I make. I wont be the only one who is working on this code though, do you think it is safe to leave the code like this. In future if someone change the code, at least pushing lots of different datatypes could break the code and other developer knows something is not right. – Mr H Mar 20 '19 at 06:35
  • Just as an example of where property testing is applicable: I am an author and a maintainer of a package [`Iteraptor`](https://hexdocs.pm/iteraptor/intro.html#iterating-mapping-reducing), that allows deep iteration/mapping/reducing of any enumerables. Here the input is absolutely undetermined, so I [build a deeply nested struct of random enumerables](https://github.com/am-kantox/elixir-iteraptor/blob/master/test/property/iteraptor_test.exs#L78-L83) and validate it works as expected. – Aleksei Matiushkin Mar 20 '19 at 06:37
  • I honestly do not see the application of property testing in this particular case. As I said, defend your code with guards, pattern matching and typespecs/dialyzer so that wrong input won’t pass to the function at all. – Aleksei Matiushkin Mar 20 '19 at 06:38
  • 1
    Hmmm, so based on what you are saying, I rarely need it for stuff that for example coming from API. Anyway, Thank you very much for your help. One thing though, Property testing has shown me the flaws in my code. I don't mind to do it so I get better at it over time. Thanks again. – Mr H Mar 20 '19 at 06:53
  • Always welcome. Yes, one rarely needs property testing for the stuff that is of known shape. It’s better to explicitly state the function expects the proper input `def my_fun(%{"id" => id}) when is_integer(id) and id > 0` and declare the sink-all clause with `def my_fun(_), do: {:error, :invalid_input}` or like, rather than test that your code does not blow up with the invalid input. For GraphQL, OTOH, it might be useful to avoid producing a ton of different unit tests. – Aleksei Matiushkin Mar 20 '19 at 06:57

0 Answers0