Couchdb: Adopt Credo inside Elixir test suite

[dottorblaster]

The Elixir test suite gets slowly broader and broader, and I think it could be a good idea to adopt Credo as the linter and static code analysis tool of choice. What do you think about it?

Ref.: https://github.com/rrrene/credo

[wohali]

/cc @jaydoane @garrensmith

[dottorblaster]

Wow, blazing fast @wohali! 🙂

[jaydoane]

Seems like a great idea to me. It was easy to add to mix.exs, and when run it generated a bunch of issues (below). My only complaint so far is that the max line length check is set to 120 by default, but it seems simple to add to something like the following to .credo.exs to change it to 80 (which I would suggest):
{Credo.Check.Readability.MaxLineLength, [priority: :low, max_length: 80]}

$ mix credo -a
Checking 17 source files ...

  Software Design
┃
┃ [D] → Found a TODO tag in a comment: # TODO: port sever_port tests from
┃       config.js
┃       test/config_test.exs:66 #(ConfigTest.delete_config)
┃ [D] → Found a TODO tag in a comment: # TODO: In JS we re-fetch _changes with
┃       since_seq, is that
┃       test/replication_test.exs:573 #(ReplicationTest.run_since_seq_repl)
┃ [D] → Found a TODO tag in a comment: # TODO: Parameterize these
┃       test/replication_test.exs:9 #(ReplicationTest)
┃ [D] → Found a TODO tag in a comment: # TODO: switch this to _local when that's
┃       landed
┃       test/config_test.exs:12 #(ConfigTest)
┃ [D] → Found a TODO tag in a comment: # TODO: port remainder of
┃       security_validation.js suite
┃       test/security_validation_test.exs:185 #(SecurityValidationTest)
┃ [D] → Found a TODO tag in a comment: # TODO: port _list function tests and
┃       everything below in rewrite.js
┃       test/rewrite_test.exs:328 #(RewriteTest)
┃ [D] → Found a TODO tag in a comment: # TODO: port the two "bugged" tests from
┃       rewrite.js
┃       test/rewrite_test.exs:318 #(RewriteTest)
┃ [D] → Found a TODO tag in a comment: # TODO: make protocol check use defined
┃       protocol value
┃       test/basics_test.exs:204 #(BasicsTest)
┃ [D] → Found a TODO tag in a comment: # TODO: enable chunked encoding
┃       test/basics_test.exs:117 #(BasicsTest)
┃ [D] → Found a TODO tag in a comment: # TODO: do we need to bring this in?
┃       test/all_docs_test.exs:11 #(AllDocsTest)

  Code Readability
┃
┃ [R] ↗ Large numbers should be written with underscores: 30_000
┃       test/replication_test.exs:1588:62 #(ReplicationTest.wait_for_repl)
┃ [R] ↗ The condition of `if` should not be wrapped in parentheses.
┃       test/replication_test.exs:1056:7 #(ReplicationTest.run_by_id_repl_impl)
┃ [R] ↗ The condition of `if` should not be wrapped in parentheses.
┃       test/replication_test.exs:1040:7 #(ReplicationTest.run_by_id_repl_impl)
┃ [R] ↗ The condition of `if` should not be wrapped in parentheses.
┃       test/replication_test.exs:1026:5 #(ReplicationTest.run_by_id_repl_impl)
┃ [R] ↗ The condition of `if` should not be wrapped in parentheses.
┃       test/replication_test.exs:1006:7 #(ReplicationTest.run_by_id_repl_impl)
┃ [R] ↗ The condition of `if` should not be wrapped in parentheses.
┃       test/replication_test.exs:991:7 #(ReplicationTest.run_by_id_repl_impl)
┃ [R] ↗ The condition of `if` should not be wrapped in parentheses.
┃       test/replication_test.exs:976:5 #(ReplicationTest.run_by_id_repl_impl)
┃ [R] ↗ Large numbers should be written with underscores: 30_000
┃       test/replication_test.exs:1608:33 #(ReplicationTest.wait_for_repl_stop)
┃ [R] ↗ Large numbers should be written with underscores: 30_000
┃       test/replication_test.exs:1355:33 #(ReplicationTest.run_continuous_repl)
┃ [R] ↗ Large numbers should be written with underscores: 30_000
┃       test/replication_test.exs:1184:30 #(ReplicationTest.run_continuous_repl)
┃ [R] ↗ The condition of `if` should not be wrapped in parentheses.
┃       test/replication_test.exs:781:7 #(ReplicationTest.run_filtered_repl)
┃ [R] ↗ The condition of `if` should not be wrapped in parentheses.
┃       test/replication_test.exs:732:7 #(ReplicationTest.run_filtered_repl)
┃ [R] ↗ Large numbers should be written with underscores: 1_000_000
┃       test/test_helper.exs:222:34 #(CouchTestCase.__using__.now)
┃ [R] ↗ Large numbers should be written with underscores: 100_000_000
┃       test/reduce_test.exs:217:36 #(ReduceTest)
┃ [R] ↗ Don't use ; to separate statements and expressions
┃       test/reduce_test.exs:89:62 #(ReduceTest)
┃ [R] ↗ Large numbers should be written with underscores: 10_000
┃       test/debug_test.exs:82:18 #(DebugTest)
┃ [R] ↗ Variable names should be written in snake_case.
┃       test/basics_test.exs:12:5 #(BasicsTest)
┃ [R] → Modules should have a @moduledoc tag.
┃       lib/couch.ex:1:11 #(Couch.Session)
┃ [R] → Space missing after comma
┃       test/reduce_test.exs:134:49 #(ReduceTest)
┃ [R] → Space missing after comma
┃       test/reduce_test.exs:133:49 #(ReduceTest)
┃ [R] → Space missing after comma
┃       test/reduce_test.exs:125:49 #(ReduceTest)
┃ [R] → Space missing after comma
┃       test/reduce_test.exs:124:49 #(ReduceTest)
┃ [R] → Space missing after comma
┃       test/reduce_test.exs:118:49 #(ReduceTest)
┃ [R] → Space missing after comma
┃       test/reduce_test.exs:117:49 #(ReduceTest)
┃ [R] → Space missing after comma
┃       test/reduce_test.exs:116:49 #(ReduceTest)
┃ [R] → Space missing after comma
┃       test/reduce_test.exs:114:49 #(ReduceTest)
┃ [R] → Space missing after comma
┃       test/reduce_test.exs:113:49 #(ReduceTest)

  Refactoring opportunities
┃
┃ [F] ↗ Avoid long quote blocks.
┃       test/test_helper.exs:8:5 #(CouchTestCase.__using__)
┃ [F] → Pipe chain should start with a raw value.
┃       test/replication_test.exs:1574 #(ReplicationTest.add_attachment)
┃ [F] → Pipe chain should start with a raw value.
┃       test/replication_test.exs:1525 #(ReplicationTest.replicate)
┃ [F] → Function is too complex (CC is 17, max is 9).
┃       test/replication_test.exs:1146:7 #(ReplicationTest.run_continuous_repl)
┃ [F] → Function body is nested too deep (max depth is 2, was 3).
┃       test/replication_test.exs:1248:11 #(ReplicationTest.run_continuous_repl)
┃ [F] → Function is too complex (CC is 11, max is 9).
┃       test/replication_test.exs:245:7 #(ReplicationTest.run_simple_repl)
┃ [F] → Function body is nested too deep (max depth is 2, was 3).
┃       test/test_helper.exs:90:13 #(CouchTestCase.__using__.set_config)
┃ [F] → Function body is nested too deep (max depth is 2, was 3).
┃       test/test_helper.exs:90:13 #(CouchTestCase.__using__.set_config)
┃ [F] → Pipe chain should start with a raw value.
┃       test/security_validation_test.exs:120 #(SecurityValidationTest)

  Warnings - please take a look
┃
┃ [W] ↗ length(resp.body()["rows"]) == 0 is expensive. Prefer Enum.empty?/1 or
┃       list == []
┃       test/all_docs_test.exs:51 #(AllDocsTest)

  Consistency
┃
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:165 #(Couch.process_arguments)
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:156 #(Couch.process_arguments)
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:137 #(Couch.handle_response)
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:136 #(Couch.handle_response)
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:135 #(Couch.handle_response)
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:134 #(Couch.handle_response)
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:133 #(Couch.handle_response)
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:132 #(Couch.handle_response)
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:131 #(Couch.handle_response)
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:130 #(Couch.handle_response)
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:123 #(Couch.handle_response)
┃ [C] ↗ There is no whitespace around parentheses/brackets most of the time, but
┃       here there is.
┃       lib/couch.ex:116 #(Couch.handle_response)
┃ [C] ↗ File has the variable name before the pattern while most of the files
┃       have the variable name after the pattern when naming parameter pattern
┃       matches
┃       test/test_helper.exs:226:15 #(CouchTestCase.__using__.rev)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:134:69 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:133:69 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:132:65 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:126:65 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:125:69 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:124:69 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:123:65 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:118:69 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:117:69 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:116:69 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:115:65 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:114:69 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:113:69 #(ReduceTest)
┃ [C] ↗ There are spaces around operators most of the time, but not here.
┃       test/reduce_test.exs:112:65 #(ReduceTest)

Please report incorrect results: https://github.com/rrrene/credo/issues

Analysis took 4.1 seconds (0.04s to load, 4.1s running checks)
124 mods/funs, found 27 consistency issues, 1 warning, 9 refactoring opportunities, 27 code readability issues, 10 software design suggestions.

Showing priority issues: ↑ ↗ →  (use `--strict` to show all issues, `--help` for options).
returned 31

[wohali]

My recommendation is:

• decide on what we should filter out - beyond just line length, some of these are unhelpful (like complaining about long quote blocks and functions being too complex, esp. in a test suite)
• fix all the errors
• add steps to install the prerequisites for this to .travis.yml (elixir)
• add a new elixir-credo target to Makefile that runs this and fails on any errors,
• add the elixir-credo target to make check
• file the PR
• when Travis is green, we can merge

[van-mronov]

Hi, all!

Since version 1.6 elixir provides built-in code formatter which refers most of code style issues.
The goal of the formatter is to automate the styling of codebases into a unique and consistent layout used across teams and the whole community.

It has several options useful for CI and development:
--check-formatted - checks that the file is already formatted.
--check-equivalent - checks if the files after formatting have the same AST as before formatting. If the ASTs are not equivalent, it is a bug in the code formatter. This option is recommended if you are automatically formatting files.

If any of the --check-* flags are given and a check fails, the formatted contents won’t be written to disk nor printed to standard output.

So, perhaps, it make sense to adopt formatter firstly.

Ref.: https://hexdocs.pm/mix/Mix.Tasks.Format.html#content

[dottorblaster]

@van-mronov yeah it is a good point, I already used mix format for tests of mine, but I didn't cover those options at all. Thanks for pointing them out!

[van-mronov]

@dottorblaster @wohali do you mind if I make a PR with steps provided by @wohali but for mix format?

[wohali]

@van-mronov not at all, sounds like a good starting place!

[dottorblaster]

@van-mronov are you crazy man, all these kind of efforts and contributions are welcome!

[iilyak]

Thank you @dottorblaster and @van-mronov. Very good idea.

[van-mronov]

Hi guys, I've created PR #1767. Should I add anybody as reviewer?

[dottorblaster]

@van-mronov thanks for the effort, that's amazing! Now I'm taking the shot for credo

[dottorblaster]

@wohali thanks for merging #1769! Now should we close this or we're going further with the discussion?

[wohali]

Closed by #1769.