Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Role Skills Controller Tests #416

Merged
merged 1 commit into from
Nov 3, 2016

Conversation

amyschools
Copy link
Contributor

What's in this PR?

Adding helpers from api_case.ex.

References

Fixes #414

Progress on: #413

Copy link
Contributor

@sbatson5 sbatson5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me 👍. Nice work!

@amyschools amyschools force-pushed the 414-roleskills-controller-test branch from 379234e to eab4979 Compare November 2, 2016 21:53
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 94.83% when pulling eab4979 on 414-roleskills-controller-test into af079d0 on develop.

json = conn |> get(path) |> json_response(200)
assert json["data"] == []
conn = conn |> get(path)
assert json_response(conn, 200)["data"] == []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep this roughly as it is in some of the closed PRs. conn |> get(path) |> json_response(200), then assign the response and assert against that.

Copy link
Contributor Author

@amyschools amyschools Nov 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this okay instead? i didn't realize there was a request_index function to help with that until just now.

describe "index" do
    test "lists all entries on index", %{conn: conn} do
    [project_skill_1, project_skill_2] = insert_pair(:project_skill)

     conn
     |> request_index
     |> json_response(200)
     |> assert_ids_from_response([project_skill_1.id, project_skill_2.id])
    end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize that either.

@@ -87,62 +46,44 @@ defmodule CodeCorps.RoleSkillControllerTest do
role = insert(:role, name: "Frontend Developer")
skill = insert(:skill, title: "test skill")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to the attributes here. Just insert(:role) will work.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.669% when pulling 04271fa on 414-roleskills-controller-test into af079d0 on develop.

conn
|> request_index
|> json_response(200)
|> assert_ids_from_response([role_skill_1.id, role_skill_2.id])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing here is weird. There are three spaces.

conn
|> get(path)
|> json_response(200)
|> assert_ids_from_response([role_skill_1.id, role_skill_2.id])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might use that request_index helper instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That created identical tests for listing all the entries on the index and filtering the index...hm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filter test should have a third item that's not included in the ids from response...maybe I should put that back in to test the filtering

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amyschools yes, absolutely.

role_skill_1 = insert(:role_skill, role: role, skill: elixir)
role_skill_2 = insert(:role_skill, role: role, skill: phoenix)
insert(:role_skill, role: role, skill: rails)
[role_skill_1, role_skill_2] = insert_pair(:role_skill)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You figured out the missing third record, so I'll just link https://github.com/code-corps/code-corps-api/pull/425/files#r86388037 as a pointer on how to write this in one line.

@amyschools amyschools force-pushed the 414-roleskills-controller-test branch from 04271fa to d235101 Compare November 3, 2016 17:08
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.153% when pulling d235101 on 414-roleskills-controller-test into f9eef9d on develop.

@amyschools amyschools force-pushed the 414-roleskills-controller-test branch from d235101 to 9b60645 Compare November 3, 2016 19:15
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 94.83% when pulling 9b60645 on 414-roleskills-controller-test into 3c24fad on develop.

@amyschools amyschools merged commit dd136c2 into develop Nov 3, 2016
@amyschools amyschools deleted the 414-roleskills-controller-test branch November 3, 2016 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants