Refactoring test suite using StreamData

[shinnokdisengir]

TODO

refactoring test suite

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.48%. Comparing base (20fa298) to head (ef71037).
Report is 119 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #919      +/-   ##
==========================================
- Coverage   68.62%   65.48%   -3.14%     
==========================================
  Files         228      146      -82     
  Lines        6667     4288    -2379     
==========================================
- Hits         4575     2808    -1767     
+ Misses       2092     1480     -612     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Annopaolo]

Also, some files are missing the copyright header

[Annopaolo]

Leftover?

[shinnokdisengir]

currently, select!(:interface) doesn't join with mapping table. It's need to be done

[Annopaolo]

What's being done here is very good. In my opinion, the overall structure of tests can be improved for enhancing isolation and clarity. My suggestion is:
- A setup_all part in which a realm - whose name is randomly generated - is created, for each test file (i.e. each ExUnit.Case). This allows for non-interference of tests when running in parallel.
- A setup part, for each test, in which tables relevant for the test are populated and truncated on exit. This allows for non-interference of tests running in the same case and using the database.
- Note: cleanup is necessary only when tests change database state. For example, a test “fail to create group with invalid name” does not need a cleanup, as no group will be added to the db; a “succeeds to create group with valid input” will need cleanup.

In general, by looking at a single test it should be possible to clearly identify what is being created, what operation is being performed and what the expected output would be.

[Annopaolo]

This use hides the setup step(s) needed to populate the db, so it is unclear where e.g. devices selected at line 39 do come from. It should be possible to clearly identify what is being created, what operation is being performed and what the expected output would be by just looking at a single test.
I find that explicitly implementing setup_all and setup in the test case helps doing so.

The same goes for the other tests on groups and interfaces.

[Annopaolo]

If tests from different cases run in different keyspaces and, inside a case, each test populates the tables it needs and truncates them on exit, you get database isolation for free, without the need to write more complex queries for deleting only some data.
A quick sketch of a possible way of handling it:

defmodule MyTest do
  use ExUnit.Case
  
  setup_all do
     create_keyspace!("keyspace_#{System.unique_integer()}")
  end
  
  setup do
    # get keyspace from context 
    insert!(keyspace, devices_fixture())
    on_exit(fn -> run!("TRUNCATE TABLE devices") end)
  end

  test "my test 1" do
  # use the `devices` table
  end
end

[shinnokdisengir]

I have already discussed this with @rbino. The point is to write as little code as possible in each test file. Cases are made to achieve this

[Annopaolo]

These two can be moved in (or called from) a setup block , instead of being in a different module

[Annopaolo]

Note that setup_all runs once before all tests in a case. In order to improve test isolation and clarity, it would be better to generate device data for each test (i.e. with setup).
The same goes also for groups and interfaces