config_service: Added tests for the auth-signin element

config_service: Added tests for the auth-signin element

A new file was created for the purpose of testing the auth-signin.html
element.

BUG=747083
Review-Url: https://codereview.chromium.org/2981413002
Committed: https://github.com/luci/luci-py/commit/1ebf96b6f9191e764964a080368b6b85140b3477

[cwpayton, 2017-07-20 19:21:02]

cwpayton@google.com changed reviewers:
+ hinoka@chromium.org, sergeyberezin@chromium.org

[cwpayton, 2017-07-20 19:21:03]

PTAL

[Sergey Berezin, 2017-07-21 01:07:49]

https://codereview.chromium.org/2981413002/diff/1/appengine/config_service/ui...
File appengine/config_service/ui/test/common/auth-signin_test.html (right):

https://codereview.chromium.org/2981413002/diff/1/appengine/config_service/ui...
appengine/config_service/ui/test/common/auth-signin_test.html:31: suite('Default
values', function() {
nit: I'd consider these values "initial", not "default" (since default usually
implies existence of non-default configuration; here it's just initialization).

Let's name the suite and tests so together they read more like a story; e.g.:

suite('auth-signin initial values are as expected', function() {
  ...
  test('signed_in is false', ...)
  test('initialized is false', ...)
  test('client_id is always present', ...)

https://codereview.chromium.org/2981413002/diff/1/appengine/config_service/ui...
appengine/config_service/ui/test/common/auth-signin_test.html:46:
expect(auth_signin.client_id).to.not.be.null;
Technically, this test really checks that the client_id attribute gets correctly
set when supplied. Therefore, it's best to check that it equal the expected
value 'test-client-id'.

In order to test that client_id is required you may also want to setup another
test-fixture without client_id and check for whatever the expected behavior is
(if it makes sense).

https://codereview.chromium.org/2981413002/diff/1/appengine/config_service/ui...
appengine/config_service/ui/test/common/auth-signin_test.html:50:
suite('Initialized actions', function () {
How about this naming:

suite('fetch-configs always fires when gapi is initialized', ...
  test('when user exists', ...)
  test('when user does not exist', ...)
  test('when user does not have access token', ...)
)

[cwpayton, 2017-07-21 16:34:59]

PTAL

https://codereview.chromium.org/2981413002/diff/1/appengine/config_service/ui...
File appengine/config_service/ui/test/common/auth-signin_test.html (right):

https://codereview.chromium.org/2981413002/diff/1/appengine/config_service/ui...
appengine/config_service/ui/test/common/auth-signin_test.html:31: suite('Default
values', function() {
On 2017/07/21 01:07:49, Sergey Berezin wrote:
> nit: I'd consider these values "initial", not "default" (since default usually
> implies existence of non-default configuration; here it's just
initialization).
> 
> Let's name the suite and tests so together they read more like a story; e.g.:
> 
> suite('auth-signin initial values are as expected', function() {
>   ...
>   test('signed_in is false', ...)
>   test('initialized is false', ...)
>   test('client_id is always present', ...)

Done.

https://codereview.chromium.org/2981413002/diff/1/appengine/config_service/ui...
appengine/config_service/ui/test/common/auth-signin_test.html:46:
expect(auth_signin.client_id).to.not.be.null;
On 2017/07/21 01:07:49, Sergey Berezin wrote:
> Technically, this test really checks that the client_id attribute gets
correctly
> set when supplied. Therefore, it's best to check that it equal the expected
> value 'test-client-id'.
> 
> In order to test that client_id is required you may also want to setup another
> test-fixture without client_id and check for whatever the expected behavior is
> (if it makes sense).

Done.

https://codereview.chromium.org/2981413002/diff/1/appengine/config_service/ui...
appengine/config_service/ui/test/common/auth-signin_test.html:50:
suite('Initialized actions', function () {
On 2017/07/21 01:07:49, Sergey Berezin wrote:
> How about this naming:
> 
> suite('fetch-configs always fires when gapi is initialized', ...
>   test('when user exists', ...)
>   test('when user does not exist', ...)
>   test('when user does not have access token', ...)
> )

Done.

[Sergey Berezin, 2017-07-24 22:40:52]

LGTM, thanks!

[cwpayton, 2017-07-24 22:53:02]

The CQ bit was checked by cwpayton@google.com

[commit-bot: I haz the power, 2017-07-24 22:53:04]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-24 22:55:35]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1500936782057530,
"parent_rev": "ade0a4c5c3cc78ac10a907ba81cc56ecd56af238", "commit_rev":
"1ebf96b6f9191e764964a080368b6b85140b3477"}

[commit-bot: I haz the power, 2017-07-24 22:55:38]

Description was changed from

==========
config_service: Added tests for the auth-signin element

A new file was created for the purpose of testing the auth-signin.html
element.

BUG=747083
==========

to

==========
config_service: Added tests for the auth-signin element

A new file was created for the purpose of testing the auth-signin.html
element.

BUG=747083

Review-Url: https://codereview.chromium.org/2981413002
Committed:
https://github.com/luci/luci-py/commit/1ebf96b6f9191e764964a080368b6b85140b3477
==========

[commit-bot: I haz the power, 2017-07-24 22:55:39]

Committed patchset #2 (id:20001) as
https://github.com/luci/luci-py/commit/1ebf96b6f9191e764964a080368b6b85140b3477