appengine/config_service/ui/test/config-ui/config-set_test.html - Issue 2959833002: config_service: add last import validation and tests

Side by Side Diff: appengine/config_service/ui/test/config-ui/config-set_test.html

Issue 2959833002: config_service: add last import validation and tests (Closed)

Patch Set: Add tests to front-page and config-set page. Created 3 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« appengine/config_service/app.yaml ('K') | « appengine/config_service/ui/static/index.html ('k') | appengine/config_service/ui/test/config-ui/front-page_test.html » ('j') | appengine/config_service/ui/test/config-ui/front-page_test.html » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!--

	2 Copyright 2017 The LUCI Authors. All rights reserved.

	3 Use of this source code is governed under the Apache License, Version 2.0

	4 that can be found in the LICENSE file.

	5 -->

	6

	7 <!doctype html>

	8 <html lang="en">

	9 <head>

	10 <meta charset="utf-8">

	11 <meta name="viewport" content="width=device-width, minimum-scale=1, initial- scale=1, user-scalable=yes">

	12

	13 <title>config-set test</title>

	14

	15 <script src="../../bower_components/webcomponentsjs/webcomponents-lite.js">< /script>

	16 <script src="../../bower_components/web-component-tester/browser.js"></scrip t>

	17

	18 <link rel="import" href="../../src/config-ui/config-set.html">

	19 </head>

	20 <body>

	21

	22 <test-fixture id="projects/infra-experimental">
	Sergey Berezin 2017/06/28 19:11:35 nit: we usually try to avoid having actual product nit: we usually try to avoid having actual production names in tests. It's OK to model your data based on the real one, but it's better to rename it to something obviously fake. E.g. "projects/up-to-date-project" or "projects/invalid-config-project". Descriptive names like that also make the test more readable, as they suggest what to expect in assertions. ayanaadylova 2017/07/05 18:02:45 Done. Show quoted text On 2017/06/28 19:11:35, Sergey Berezin wrote: > nit: we usually try to avoid having actual production names in tests. It's OK to > model your data based on the real one, but it's better to rename it to something > obviously fake. E.g. "projects/up-to-date-project" or > "projects/invalid-config-project". > > Descriptive names like that also make the test more readable, as they suggest > what to expect in assertions. Done. ayanaadylova 2017/07/06 22:51:43 Done. Show quoted text On 2017/06/28 19:11:35, Sergey Berezin wrote: > nit: we usually try to avoid having actual production names in tests. It's OK to > model your data based on the real one, but it's better to rename it to something > obviously fake. E.g. "projects/up-to-date-project" or > "projects/invalid-config-project". > > Descriptive names like that also make the test more readable, as they suggest > what to expect in assertions. Done.
	23 <template>

	24 <config-set category="projects"

	25 name="infra-experimental"></config-set>

	26 </template>

	27 </test-fixture>

	28

	29 <test-fixture id="projects/infra-experimental/refs/heads/master">

	30 <template>

	31 <config-set category="projects"

	32 name="infra-experimental/refs/heads/master"></config-set>

	33 </template>

	34 </test-fixture>

	35

	36 <script>

	37 suite('<config-set>', function() {

	38 var config_set;

	39 setup(function() {

	40 config_set = fixture('projects/infra-experimental');

	41 });

	42

	43 test('checks the category of config set', function() {
	Sergey Berezin 2017/06/28 19:11:35 nit: it's a good practice to name suites and tests nit: it's a good practice to name suites and tests so they sound like English sentences. E.g.: "Up-to-date project config set" - has correct category - has correct name - sets isLoading by default - displays valid last import attempt etc. Ideally, a test suite would also read like a "story" (there is even a term "test story") - where a sequence of events leads to expected assertions at each point. See https://youtu.be/_9qARcdCAn4?t=412 for another naming sample. Having said that, this is not a hard requirement, only a guideline. ayanaadylova 2017/07/06 22:51:43 Done. Show quoted text On 2017/06/28 19:11:35, Sergey Berezin wrote: > nit: it's a good practice to name suites and tests so they sound like English > sentences. E.g.: > "Up-to-date project config set" > - has correct category > - has correct name > - sets isLoading by default > - displays valid last import attempt > etc. > > Ideally, a test suite would also read like a "story" (there is even a term "test > story") - where a sequence of events leads to expected assertions at each point. > > > See https://youtu.be/_9qARcdCAn4?t=412 for another naming sample. > > Having said that, this is not a hard requirement, only a guideline. Done.
	44 assert.equal(config_set.category, 'projects');

	45 });

	46

	47 test('checks the name of config set', function() {

	48 assert.equal(config_set.name, 'infra-experimental');

	49 });

	50

	51 test('checks that isLoading property is true by default', function() {

	52 assert.equal(config_set.isLoading, true);

	53 });

	54

	55 test('gets iron-ajax response', function () {

	56 var ajax = config_set.shadowRoot.querySelector('iron-ajax');

	57 ajax.generateRequest();

	58 ajax.addEventListener('response', function() {

	59 assert.equal(config_set.isLoading, false);

	60 assert.equal(config_set.files.length, 11);

	61 assert.equal(config_set.lastImportAttempt.success, true);

	62 assert.notEqual(config_set.location, "");

	63 });

	64 });

	65

	66 });

	67

	68 suite('<config-set>', function() {
	Sergey Berezin 2017/06/28 19:11:35 Should the suite names be different? The two test Should the suite names be different? The two test suites seem to be running roughly the same tests for different config sets, which seems redundant. Since your change is about detecting valid vs. invalid configs, it makes sense to have tests for these two cases. I'd suggest to turn this second suite into tests for an invalid config set. ayanaadylova 2017/07/06 22:51:43 Done. Show quoted text On 2017/06/28 19:11:35, Sergey Berezin wrote: > Should the suite names be different? > > The two test suites seem to be running roughly the same tests for different > config sets, which seems redundant. > > Since your change is about detecting valid vs. invalid configs, it makes sense > to have tests for these two cases. I'd suggest to turn this second suite into > tests for an invalid config set. Done.
	69 var config_set;

	70 setup(function() {

	71 config_set = fixture('projects/infra-experimental/refs/heads/master');

	72 });

	73

	74 test('checks the category of config set', function() {

	75 assert.equal(config_set.category, 'projects');

	76 });

	77

	78 test('checks the name of config set', function() {

	79 assert.equal(config_set.name, 'infra-experimental/refs/heads/master');

	80 });

	81

	82 test('checks that isLoading property is true by default', function() {

	83 assert.equal(config_set.isLoading, true);

	84 });

	85

	86 test('gets iron-ajax response', function () {

	87 var ajax = config_set.shadowRoot.querySelector('iron-ajax');

	88 ajax.generateRequest();

	89 ajax.addEventListener('response', function() {

	90 assert.equal(config_set.isLoading, false);

	91 assert.equal(config_set.files.length, 0);
	Sergey Berezin 2017/06/28 19:11:35 nit: it would be interesting to generate a few fil nit: it would be interesting to generate a few files in a config set, here or in another test, for better coverage. ayanaadylova 2017/07/06 22:51:43 Done. Show quoted text On 2017/06/28 19:11:35, Sergey Berezin wrote: > nit: it would be interesting to generate a few files in a config set, here or in > another test, for better coverage. Done.
	92 assert.equal(config_set.lastImportAttempt.success, true);

	93 assert.notEqual(config_set.location, "");

	94 });

	95 });

	96

	97 });

	98

	99 </script>

	100 </body>

	101 </html>

OLD	NEW