Clean Up and Dry Tests #344

eliasjpr · 2017-10-30T20:20:15Z

Description of the Change

As the framework grows having a clean and expressive test suite is very important for those who are currently working on the project and new people that want to contribute.

This PR is a WORK IN PROGRESS to clean up and structure the test using the Arrange, Act, Assert Annihilate pattern with only one action and one assertion.

Alternate Designs

Currently the test suite is just testing each class part of the Amber Framework maybe it would be a good idea to create 2 directories for integrations, unit tests

Benefits

With the work done it will make it very easy to read and maintain the test suite

Possible Drawbacks

Maybe there is a better design outther

marksiemers · 2017-10-30T20:39:21Z

spec/support/fixtures/cli_fixtures.cr

+
+CONT
+  end
+end


Crystal will create whitespace based on the closing CONT, so you can do:

module CLIFixtures def expected_controller <<-CONT class AnimalController < ApplicationController def add render("add.slang") end def list render("list.slang") end def remove render("remove.slang") end end CONT end end

Yeah I was confused about this, fixing it

The closing CONT should align with class and the last end

marksiemers · 2017-10-30T20:51:18Z

@eliasjpr Thanks for your work on this.

I like a lot of the refactoring so far (particularly scaffold_app)

My main concern is moving should statements into helpers. Crystal's Spec definitely has taken cues from RSpec, and there, the convention is to have should statements directly in an it block. (Well in RSpec it is expect now).

eliasjpr · 2017-10-30T21:23:18Z

I agree when having 1 assertion line, but when there is multiple line attempting to assert related expectations we should have them in a composed assert method, this makes it very reusable, readable and follow the one assert rule. IMHO

An example of this is when asserting if the encryption files exists after encrypting.

File.exists?("config/environments/#{enc_file}").should be_true
File.read(enc_key).size.should eq 44

marksiemers · 2017-10-30T22:35:49Z

I think "Assert" in the AAA pattern is roughly equal to should in Spec, if you move two shoulds into a helper, IMHO it is just hiding that there are two asserts.

I think the idea behind the one assertion per test, is to make each test (it) statement as granular as possible.

Using this spec file as a starting point

describe MainCommand::Encrypt do
  context "application structure" do
    it "creates amber directory structure" do
      scaffold_app(TESTING_APP)
      MainCommand.run ["encrypt", "test"]
      assert_encrypted_files_exists?(".test.enc", ".amber_secret_key")
      cleanup
    end
  end
end

The it statement seems broad, and does not match what the assertion is actually checking.

I would like to see that our goal is to have each it only execute one should (directly or indirectly). For example, I find the following code more readable and I have a better understanding of what amber encrypt test should do:

describe MainCommand::Encrypt do
  begin
    scaffold_app(TESTING_APP)
    arg = "test"
    MainCommand.run ["encrypt", arg]

    it "should create an hidden .[arg].enc file" do
      File.exists?("config/environments/.#{arg}.enc").should be_true
    end

    it "should create a 44 character secret key in .amber_secret_key" do
      File.read(".amber_secret_key").size.should eq 44
    end

  ensure
    cleanup
  end
end

I know it isn't always practical to have a 1:1 it to should ratio, but I think using it as a guide can lead to more declarative and descriptive tests.

eliasjpr · 2017-10-30T22:43:40Z

Thats what I am looking for! Thanks @marksiemers

marksiemers · 2017-10-30T22:46:51Z

It isn't bad practice to setup descriptions for all your known requirements right away:

module Amber::CLI
  describe MainCommand::Perform do
    it "performs a tasks via command line" {} # `it` requires a block

    it "performs a tasks via command line alias" {}

    it "shows task description" {}
  end
end

marksiemers

@eliasjpr - Overall, this is great. The organization into helpers and fixtures looks really good.

I have some inline comments about renaming/restructuring the outlines of some of the specs, but again, this mostly looks great.

marksiemers · 2017-10-31T01:13:32Z

spec/amber/cli/commands/generator_spec.cr

+
+    context "when scaffolding an Amber app" do
+      scaffold_app(TESTING_APP)
+      MainCommand.run ["generate", "scaffold", "Animal", "name:string"]


This line shouldn't be needed to test a shards build - did we just want something in there?

If we want a more complete test, it should be structured something like this:

context "scaffolding an Amber app" assert that it builds test scaffold generator (in a describe block) test controller generator (in a describe block) test model generator (in a describe block) ...test each generator... assert that it still builds cleanup end

Practically speaking, we may not want two builds, until the shard downloading thing is optimized one way or another.

It might be better to test the shard building at the end - will it build after all the generators have been run, or did any of them introduce a compile time error?

marksiemers · 2017-10-31T01:15:29Z

spec/amber/cli/commands/generator_spec.cr

+      cleanup
+    end
+
+    context "when generating controllers" do


Semantically, I would say we are in the context of "scaffolding and amber app" and this should be describe "amber generate controller"

marksiemers · 2017-10-31T01:28:33Z

spec/amber/cli/commands/generator_spec.cr

+      MainCommand.run ["generate", "controller", controller] | options
+
+      it "generates controller with correct verbs and actions" do
+        File.read("./src/controllers/#{controller.downcase}_controller.cr").should eq expected_controller


This line and line 25 are coupling this code with wherever expected_controller is defined. I would say choose one place and have both controller = "Animal" and whatever is in the definition of expected_controller. I think the helper or the fixture is the better place.

For an example of how I approached it, see here: 3220d13

And the helper here: https://github.com/marksiemers/amber/blob/3220d1312608f53ca486b8f5a668219870c19b57/spec/amber/cli/templates/migration_spec_helper.cr

All the decisions about names are in the helper - so they are easier to keep in sync.

marksiemers · 2017-10-31T01:36:48Z

spec/support/fixtures/render_fixtures.cr

+  end
+
+  def layout_with_template
+    %(<html>\n  <body>\n    <h1>Hello World</h1>\n<p>I am glad you came</p>\n  </body>\n</html>)


Any reason for this not to use a heredoc like the others?

marksiemers · 2017-10-31T01:41:46Z

spec/amber/cli/commands/init_spec.cr

-          Amber::CLI::Spec.cleanup
-        end
+  describe MainCommand::New do
+    context "application structure" do


I would call this describe "amber new #{TESTING_APP}"

marksiemers · 2017-10-31T01:43:08Z

spec/amber/cli/commands/init_spec.cr

+    end
+
+    context "template" do
+      cleanup


Should this be necessary?

marksiemers · 2017-10-31T01:45:54Z

spec/amber/controller/base_spec.cr

-        HTML
-
-        TestController.new(context).render_with_csrf.should eq html_output
+        TestController.new(context).render_with_csrf.should eq csrf_form


All of this looks so much better with the refactor, nice work.

marksiemers · 2017-10-31T01:47:18Z

spec/support/fixtures/cli_fixtures.cr

+  end
+end
+
+CONT


As long as you move the closing CONT over, you can safely indent the string.

marksiemers · 2017-10-31T01:48:24Z

spec/support/helpers.cr

+  include CookieHelper
+  include RouterHelper
+  include WebsocketsHelper
+  include ValidationsHelper


I really like this organization.

This organizes and cleans up and DRY the CLI specs making it more readable and cohesive.

marksiemers · 2017-11-01T22:06:19Z

spec/amber/cli/commands/encrypt_spec.cr

-          Amber::CLI::Spec.cleanup
-        end
-      end
+  describe MainCommand::Encrypt do


Any opinion on having this be describe "amber encrypt" do ?

marksiemers

See inline comments, the name changes for the describe/context blocks are optional.

If the assert methods are not being used anymore, they should be removed.

marksiemers · 2017-11-01T22:07:28Z

spec/amber/cli/commands/generator_spec.cr


-          `shards build`
+module Amber::CLI
+  describe MainCommand::Generate do


Same thing here - we could have describe "amber generate" do and remove the context below.

marksiemers · 2017-11-01T22:10:17Z

spec/amber/cli/commands/init_spec.cr

-          Amber::CLI::Spec.amber_yml["language"].should eq "slang"
-          File.read("#{TESTING_APP}/.amber_secret_key").size.should eq 44
-          Amber::CLI::Spec.cleanup
+  describe MainCommand::New do


Same thing here, describe "amber new #{TESTING_APP}" do and remove the context below.

marksiemers · 2017-11-01T22:11:01Z

spec/amber/cli/commands/init_spec.cr

+        dirs(TESTING_APP).sort.should eq dirs(APP_TEMPLATE_PATH).sort
+      end
+
+      describe "-m granite (Granite ORM)" do


I would call this context but it isn't a deal-breaker.

marksiemers · 2017-11-01T22:11:13Z

spec/amber/cli/commands/init_spec.cr

-            Amber::CLI::Spec.db_yml["pg"].should_not be_nil
-            db_url = Amber::CLI::Spec.db_yml["pg"]["database"].as_s
-            db_url.should_not be_nil
+      describe "-m crecto (Crecto Repo)" do


Same thing, context

marksiemers · 2017-11-01T22:13:45Z

spec/support/helpers/cli_helper.cr

+  def assert_encrypted_files_exists?(enc_file, enc_key)
+    File.exists?("config/environments/#{enc_file}").should be_true
+    File.read(enc_key).size.should eq 44
+  end


Are these still needed?

drujensen

Seems to be a lot cleaner but didn't spend a ton of time reviewing since its such a large PR.

* Organize Factories into Fixtures * Organizes Helpers into submodules * Cleans up the CLI Commands Specs This organizes and cleans up and DRY the CLI specs making it more readable and cohesive. * Updates * fixup! Organize Factories into Fixtures * fixup! Organize Factories into Fixtures * fixup! Organize Factories into Fixtures

* Organize Factories into Fixtures * Organizes Helpers into submodules * Cleans up the CLI Commands Specs This organizes and cleans up and DRY the CLI specs making it more readable and cohesive. * Updates * fixup! Organize Factories into Fixtures * fixup! Organize Factories into Fixtures * fixup! Organize Factories into Fixtures Former-commit-id: b6f0459

eliasjpr added the WIP label Oct 30, 2017

eliasjpr requested review from a team October 30, 2017 20:20

faustinoaq approved these changes Oct 30, 2017

View reviewed changes

marksiemers reviewed Oct 30, 2017

View reviewed changes

eliasjpr force-pushed the ep/clean-tests branch 3 times, most recently from 070aac6 to 55e81b4 Compare October 31, 2017 00:41

marksiemers reviewed Oct 31, 2017

View reviewed changes

marksiemers mentioned this pull request Oct 31, 2017

[WIP] Implement error generator (#294) #345

Closed

Elias J. Perez added 3 commits October 31, 2017 18:54

Organize Factories into Fixtures

a0f356a

Organizes Helpers into submodules

c9924cf

Cleans up the CLI Commands Specs

ff9b2da

This organizes and cleans up and DRY the CLI specs making it more readable and cohesive.

eliasjpr force-pushed the ep/clean-tests branch from 64e053b to fad1d3c Compare November 1, 2017 01:56

Updates

5483e9e

eliasjpr force-pushed the ep/clean-tests branch 4 times, most recently from a85af50 to 0b6caad Compare November 1, 2017 15:28

eliasjpr changed the title ~~[WIP] Clean Up and Dry Tests~~ Clean Up and Dry Tests Nov 1, 2017

eliasjpr removed the WIP label Nov 1, 2017

fixup! Organize Factories into Fixtures

bc40797

eliasjpr force-pushed the ep/clean-tests branch from 0b6caad to bc40797 Compare November 1, 2017 16:02

faustinoaq mentioned this pull request Nov 1, 2017

Update dependencies #349

Merged

fixup! Organize Factories into Fixtures

192fd63

faustinoaq approved these changes Nov 1, 2017

View reviewed changes

marksiemers reviewed Nov 1, 2017

View reviewed changes

marksiemers suggested changes Nov 1, 2017

View reviewed changes

eliasjpr force-pushed the ep/clean-tests branch from 4b1088c to 6023245 Compare November 1, 2017 22:45

marksiemers approved these changes Nov 1, 2017

View reviewed changes

fixup! Organize Factories into Fixtures

7425718

eliasjpr force-pushed the ep/clean-tests branch from 6023245 to 7425718 Compare November 1, 2017 22:54

drujensen approved these changes Nov 1, 2017

View reviewed changes

eliasjpr merged commit 5649619 into master Nov 1, 2017

eliasjpr deleted the ep/clean-tests branch November 2, 2017 18:55

eliasjpr mentioned this pull request Nov 4, 2017

Test suite takes too long to run #342

Closed

This was referenced Nov 12, 2017

Document merged PRs from the last month. #384

Closed

Pull Requests which need to be documented before release. #385

Closed

faustinoaq added this to Done in Framework 2018 May 5, 2018

faustinoaq removed this from Done in Framework 2018 Jun 12, 2018

faustinoaq added this to Done in Amber Framework 2017 Jun 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean Up and Dry Tests #344

Clean Up and Dry Tests #344

eliasjpr commented Oct 30, 2017

marksiemers Oct 30, 2017 •

edited

eliasjpr Oct 30, 2017

marksiemers Oct 30, 2017

marksiemers commented Oct 30, 2017

eliasjpr commented Oct 30, 2017

marksiemers commented Oct 30, 2017 •

edited

eliasjpr commented Oct 30, 2017

marksiemers commented Oct 30, 2017

marksiemers left a comment

marksiemers Oct 31, 2017

marksiemers Oct 31, 2017

marksiemers Oct 31, 2017

marksiemers Oct 31, 2017

marksiemers Oct 31, 2017

marksiemers Oct 31, 2017

marksiemers Oct 31, 2017

marksiemers Oct 31, 2017

marksiemers Oct 31, 2017

marksiemers Oct 31, 2017

marksiemers Oct 31, 2017

marksiemers Nov 1, 2017

marksiemers left a comment

marksiemers Nov 1, 2017

marksiemers Nov 1, 2017

marksiemers Nov 1, 2017

marksiemers Nov 1, 2017

marksiemers Nov 1, 2017

drujensen left a comment

+                end
+              end
+              CONT

Clean Up and Dry Tests #344

Clean Up and Dry Tests #344

Conversation

eliasjpr commented Oct 30, 2017

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

marksiemers Oct 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marksiemers commented Oct 30, 2017

eliasjpr commented Oct 30, 2017

marksiemers commented Oct 30, 2017 • edited

eliasjpr commented Oct 30, 2017

marksiemers commented Oct 30, 2017

marksiemers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marksiemers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drujensen left a comment

Choose a reason for hiding this comment

marksiemers Oct 30, 2017 •

edited

marksiemers commented Oct 30, 2017 •

edited