{"action":"created","issue":{"url":"https://api.github.com/repos/hypothesis/h/issues/2116","labels_url":"https://api.github.com/repos/hypothesis/h/issues/2116/labels{/name}","comments_url":"https://api.github.com/repos/hypothesis/h/issues/2116/comments","events_url":"https://api.github.com/repos/hypothesis/h/issues/2116/events","html_url":"https://github.com/hypothesis/h/pull/2116","id":65443011,"number":2116,"title":"Simplify auth-controller-test.coffee","user":{"login":"seanh","id":22498,"avatar_url":"https://avatars.githubusercontent.com/u/22498?v=3","gravatar_id":"","url":"https://api.github.com/users/seanh","html_url":"https://github.com/seanh","followers_url":"https://api.github.com/users/seanh/followers","following_url":"https://api.github.com/users/seanh/following{/other_user}","gists_url":"https://api.github.com/users/seanh/gists{/gist_id}","starred_url":"https://api.github.com/users/seanh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/seanh/subscriptions","organizations_url":"https://api.github.com/users/seanh/orgs","repos_url":"https://api.github.com/users/seanh/repos","events_url":"https://api.github.com/users/seanh/events{/privacy}","received_events_url":"https://api.github.com/users/seanh/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2015-03-31T12:51:56Z","updated_at":"2015-03-31T13:11:44Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/hypothesis/h/pulls/2116","html_url":"https://github.com/hypothesis/h/pull/2116","diff_url":"https://github.com/hypothesis/h/pull/2116.diff","patch_url":"https://github.com/hypothesis/h/pull/2116.patch"},"body":"- Remove variables $scope, $timeout, auth, session and controller that were\r\ninitialized in beforeEach() functions and shared between tests.\r\n\r\n- Instead add a factory function (_controller()) that creates and returns\r\nthese variables, have each test that needs them call it to get just the\r\nones it needs.\r\n\r\n- Remove the beforeEach() that was $providing a bunch of variables to the\r\nAngular injector, it's not needed anymore.\r\n\r\n- We don't need $provide() at all anymore, and we use inject() only to inject\r\n$rootScope and $controller (which I think is unavoidable). This helps to\r\ndecouple our tests from Angular and make them easier to follow, without\r\nhaving to understand the Angular injector.\r\n\r\n- Only set the session spy in tests that need it\r\n\r\nThis decouples the tests from each other by removing shared variables and\r\nshared beforeEach() behavior, making it easier to modify tests or add new\r\ntests without affecting existing tests.\r\n\r\nIf a test doesn't need a particular variable, fixture or piece of setup code -\r\nit just doesn't call it (also saves CPU cycles).\r\n\r\nIt also makes the tests easier to read - each test can be more easily\r\nunderstood by just looking at its own code, without having to understand how\r\na bunch of global variables are created by Angular providing and injecting.\r\nEach test function declares all the variable names that it uses inside its\r\nown function code."},"comment":{"url":"https://api.github.com/repos/hypothesis/h/issues/comments/88078791","html_url":"https://github.com/hypothesis/h/pull/2116#issuecomment-88078791","issue_url":"https://api.github.com/repos/hypothesis/h/issues/2116","id":88078791,"user":{"login":"seanh","id":22498,"avatar_url":"https://avatars.githubusercontent.com/u/22498?v=3","gravatar_id":"","url":"https://api.github.com/users/seanh","html_url":"https://github.com/seanh","followers_url":"https://api.github.com/users/seanh/followers","following_url":"https://api.github.com/users/seanh/following{/other_user}","gists_url":"https://api.github.com/users/seanh/gists{/gist_id}","starred_url":"https://api.github.com/users/seanh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/seanh/subscriptions","organizations_url":"https://api.github.com/users/seanh/orgs","repos_url":"https://api.github.com/users/seanh/repos","events_url":"https://api.github.com/users/seanh/events{/privacy}","received_events_url":"https://api.github.com/users/seanh/received_events","type":"User","site_admin":false},"created_at":"2015-03-31T13:11:44Z","updated_at":"2015-03-31T13:11:44Z","body":"I feel like we shouldn't really need to inject `$rootScope.$new()` in these tests either, we should just be able to pass a `{}` for the scope for testing, but we can't because `AuthController` access a lot of properties (and nested properties) and methods of `$scope`, and it doesn't look like it creates any of these properties itself.\r\n\r\nUsing `$scope` in this way seems like a pretty nasty anti-pattern to me. Into `AuthController` tightly coupled to whatever code is creating all these `$scope` properties like `.account.tab`, `.form`, etc?\r\n\r\nWonder if we should use the \"controller as\" pattern instead."}}