Action item test #390

nicholalexander · 2018-02-26T21:16:10Z

Description of Change(s) Introduced:
An e2e test which exercises reassigning an action item from an initial user to another user. This uses three new test helpers, persist_other_user_for_retro, assign_idea, and set_participation to get the initial state correct. It includes a first test to verify the action item is present and assigned to Other User (the non primary test user). It then exercises the idea edit form and checks that it has been reassigned.

This also sets up a new other_user object in the test config.

It might make sense to clean up some of the helpers to generalize the persist_user_for_retro but wasn't sure how I'd do that - still figuring out the context. Also, didn't update the tags on this test as I wasn't sure how you might want that set.

refactored with @laumontemayor

Relevant github Issue:
Resolves #366

issue 366

Thanks!

vanderhoop · 2018-02-27T00:15:33Z

test/support/test_helpers.ex

+    Map.put(context, :other_user, other_user)
+  end
+
+  def assign_idea(%{other_user: other_user, retro: retro} = context) do


hmmm... rather than hardcode an action items values here, i think we should alter the existing persist_idea_for_retro function such that it can be assigned to different users depending on the needs of the test. That way, we encapsulate idea creation in a single helper that can take dynamic category, body values, assignee, and so on

yes, i agree! will update.

vanderhoop · 2018-02-27T00:19:52Z

test/support/test_helpers.ex

@@ -3,6 +3,7 @@ defmodule RemoteRetro.TestHelpers do
  alias RemoteRetro.{Repo, User, Vote}


To avoid repeating RemoteRetro, you can leverage the alias macro, seen here.

vanderhoop · 2018-02-27T00:21:10Z

test/support/test_helpers.ex

+  end
+
+  def assign_idea(%{other_user: other_user, retro: retro} = context) do
+    idea = %RemoteRetro.Idea{assignee_id: other_user.id, body: "blurgh", category: "action-item", retro_id: retro.id, user_id: other_user.id} |> RemoteRetro.Repo.insert!


Hey Nichol, tiny but helpful elixir tidbit is that you can leverage the alias macro (see comment) above so that you don't have to include the RemoteRetro module prefix repeatedly. To learn more, peep this!

vanderhoop · 2018-02-27T00:33:36Z

test/support/test_helpers.ex

@@ -36,6 +37,27 @@ defmodule RemoteRetro.TestHelpers do
    Map.put(context, :user, user)
  end

+  def persist_other_user_for_retro(context) do


hey @nicholalexander, i see what you mean about wanting to generalize user creation logic. given that each test helper takes and returns a context, maybe we could simply change the existing persist_user_for_retro to persist_users_for_retro, and have the callsites define @users as a list of users. That way, we can keep the logic in the existing (though altered) method and avoid introducing a new method just for this 'other user' case

vanderhoop · 2018-02-27T00:35:39Z

test/support/test_helpers.ex

+    Map.put(context, :idea, idea)
+  end
+
+  def set_participation(%{other_user: other_user, retro: retro} = context) do


given that a participation record represents the link of a user to a retro, i think we might want to encapsulate the creation of a participation in the persist_users_for_retro helper. the benefit is that then we don't have to call set_participation after we've called the helper that builds the necessary user. Whaddaya think?

the only reason not to do this was if we wanted to test users without participations - like potentially if a user was invited but never joined and was assigned an action item? or something wierd like that? Not sure. Might be overly convoluted. I think I'll just set it in persist_users as you suggest and then if its a problem down the road we can split it out.

vanderhoop · 2018-02-27T00:38:13Z

browser_logs.log

@@ -0,0 +1,16 @@
+%cDownload the React DevTools for a better development experience: https://fb.me/react-devtools


we recently added browser_logs.log to the .gitignore, so i think this branch may have been cut off of master before that change got in. @nicholalexander Can you rebase and repush to see if this file gets removed from the diff?

yes! should'a seen this.

coveralls · 2018-03-01T19:28:14Z

Coverage remained the same at 96.703% when pulling 9aac1e0 on action_item_test into 992d6bc on master.

nicholalexander · 2018-03-01T19:36:35Z

This refactors the persist_idea and persist_users to a general case but tries to keep previous use the same or as close to the same as possible. I think overall, it's much clearer, except maybe the if else in persist_idea_for_retro - the else clause is the old way of doing which makes it so that the caller doesn't have to separately define an assignee.

I think this should be ok, and if the logic we use to assign ideas becomes more complex, we can revist how this works.

Primarily, you should check how things get called in the most complicated case, when an action-item is reassigned in retro_idea_realtime_update_test.exs.

Additionally this adds Apex to the dependencies which is Elixir's version of Awesome Print.

vanderhoop · 2018-03-01T22:02:23Z

config/test.exs

+config :remote_retro, :other_user, %{
+  "email" => "misstestuser@gmail.com",
+  "email_verified" => "true", "family_name" => "Alexander",
+  "gender" => "female", "given_name" => "Nicole",


Nicole, eh? 👍

thought we'd want to have a good selection of test users.

vanderhoop · 2018-03-01T22:10:07Z

test/support/test_helpers.ex

+    Enum.each(users, fn user ->
+      persist_user(user)
+    end)
+    persisted_users = User |> Repo.all 


hey @nicholalexander! your persist_user helper returns a persisted user (with id) so here where you're doing Enum.each, you could instead do Enum.map and avoid the extra call to the db with User |> Repo.all

I looked at this but there was a problem with the travis user sometimes already being in the database and couldn't quite figure out how that was happening. i'm updating the persist to be an insert or find.

vanderhoop · 2018-03-02T14:56:49Z

test/support/test_helpers.ex

+    String.replace(name, ~r/ +/, "") |> Macro.underscore |> String.to_atom
+  end
+
+  defp user_map(users) do


I think we can do away with user_name_atom helper, as Maps can house string keys. This avoids the need for the helper, as well as the transformations done when building the map and accessing it. Separately, I think we should use the users email as the key in this map, rather than the user's name, as email is unique.

i thought about that but i was afraid that would make the call sites in retro_channel_test.exs sort of wierd that you'd have to know the email address of your mock user to use it in the tests.

also, i liked having the atom...

if anything i was thinking that we should refactor mock_user to be test_user so it's consistent with the name - or else change mock_user's name from test_user to mock_user (same difference really).

I don't feel particularly strongly so up to you!

you'd have to know the email address of your mock user to use it in the tests

Good point. What if we changed the test users' names and emails to be something semantic, like @test_user_one, where the email is testuser@one.com, and @test_user_two, where the email is testuser@two.com. We could do the same thing with the names, as I think that would make things much clearer for all folks who ending up reading/writing tests?

sounds good. i'll make that change.

made the change but left the names "Test User" and "Other User" because i felt it made it easier to read the error messages on failing tests. I think it clarifies things, though. This should be good to merge now. Lemme know!

vanderhoop · 2018-03-02T14:58:35Z

test/support/test_helpers.ex

+      persist_user(user)
+    end)
+    persisted_users = User |> Repo.all 
+    Map.merge(user_map(persisted_users), context)


we'll want to reverse the arguments here, as we don't want any users on context taking precedent over the users you just persisted, as they'll have the ids we want

vanderhoop · 2018-03-02T15:07:10Z

test/support/test_helpers.ex

+  end
+
+  defp persist_assigned_idea(user, idea, retro) do
+    %Participation{retro_id: retro.id, user_id: user.id} |> Repo.insert!


Hmmm... our application adds a participation record when the user joins a retro, so I think that the participation record should be persisted in the persist_users_for_retro helper, as it will confused anyone who doesn't have intimate knowledge of the system would be confused as to why a participation is being created as an implicit pre-req to the action item being assigned

vanderhoop · 2018-03-02T15:15:23Z

test/features/retro_idea_realtime_update_test.exs

@@ -87,4 +92,33 @@ defmodule RetroIdeaRealtimeUpdateTest do
      assert String.contains?(action_items_list_text, "let's do the thing! (Test User)")
    end
  end
+
+  describe "when an action-item is reassigned" do


test language here can be made a clearer: "when an action item is assigned"-> "it can be reassigned to another user"

clearer test language. insert_or_find behavior use map to avoid database query switch context argument order for map.

nicholalexander requested a review from vanderhoop February 26, 2018 21:47

vanderhoop reviewed Feb 27, 2018

View reviewed changes

nicholalexander added 15 commits March 1, 2018 12:21

mock_user_2 and outline for where new test goes

a901ff3

wip

7975836

a failing test! wooooo

bbfd272

working test! that actually tests things!

d476112

refactor of test and use 'other_user' consistently.

e97eee6

refactor tests

30c04b3

working refactor.

b7d96ed

remove commented out code.

3837946

use Apex for awesome printing

72b0f06

working idea with assignee and creator

65a5ae1

refactor other call sites and test helpers

0718a7b

remove iex

6cab4d9

remove unused variables

de036a2

wip

a159fee

update call sites for retro_channel_test

5d7b700

nicholalexander force-pushed the action_item_test branch from b0a2924 to 5d7b700 Compare March 1, 2018 19:21

nicholalexander changed the title ~~Action item test~~ WIP: Action item test Mar 1, 2018

remove old placeholde methods.

ee119e9

nicholalexander changed the title ~~WIP: Action item test~~ Action item test Mar 1, 2018

vanderhoop reviewed Mar 1, 2018

View reviewed changes

vanderhoop reviewed Mar 2, 2018

View reviewed changes

nicholalexander added 2 commits March 2, 2018 10:46

update based on pr feedback.

0994324

clearer test language. insert_or_find behavior use map to avoid database query switch context argument order for map.

refactor mock user names and emails.

9aac1e0

vanderhoop merged commit 2e6c4e4 into master Mar 3, 2018

vanderhoop mentioned this pull request Mar 3, 2018

Revert "Action item test" #393

Closed

vanderhoop deleted the action_item_test branch March 3, 2018 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Action item test #390

Action item test #390

nicholalexander commented Feb 26, 2018

vanderhoop Feb 27, 2018

nicholalexander Feb 27, 2018

vanderhoop Feb 27, 2018

nicholalexander Feb 27, 2018

vanderhoop Feb 27, 2018

nicholalexander Feb 27, 2018

vanderhoop Feb 27, 2018

nicholalexander Feb 27, 2018

vanderhoop Feb 27, 2018

nicholalexander Feb 27, 2018

vanderhoop Feb 27, 2018 •

edited

Loading

vanderhoop Feb 27, 2018

nicholalexander Feb 27, 2018

coveralls commented Mar 1, 2018 •

edited

Loading

nicholalexander commented Mar 1, 2018

vanderhoop Mar 1, 2018

nicholalexander Mar 2, 2018

vanderhoop Mar 1, 2018

nicholalexander Mar 2, 2018

vanderhoop Mar 2, 2018

nicholalexander Mar 2, 2018

vanderhoop Mar 2, 2018

nicholalexander Mar 2, 2018

nicholalexander Mar 2, 2018

vanderhoop Mar 2, 2018 •

edited

Loading

nicholalexander Mar 2, 2018

vanderhoop Mar 2, 2018

nicholalexander Mar 2, 2018

vanderhoop Mar 2, 2018

nicholalexander Mar 2, 2018

		@@ -3,6 +3,7 @@ defmodule RemoteRetro.TestHelpers do
		alias RemoteRetro.{Repo, User, Vote}

		@@ -0,0 +1,16 @@
		%cDownload the React DevTools for a better development experience: https://fb.me/react-devtools

Action item test #390

Action item test #390

Conversation

nicholalexander commented Feb 26, 2018

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

vanderhoop Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 1, 2018 • edited Loading

nicholalexander commented Mar 1, 2018

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

vanderhoop Mar 2, 2018 • edited Loading

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

vanderhoop Feb 27, 2018 •

edited

Loading

coveralls commented Mar 1, 2018 •

edited

Loading

vanderhoop Mar 2, 2018 •

edited

Loading