Add BDD test #80

naogify · 2021-12-09T08:19:10Z

Close #54

Description

Sorry for the big pull request.

I have implemented the following features

Added integration tests.
Added the --headlessoption to test charites serve

if you run charites serve style.yml --headless, charites will skip opening browser. I added this option because I didn't want to open the browser while the test was running. If there is a better way, please let me know.

The implementation part is as follows

charites/src/commands/serve.ts

Lines 88 to 91 in 59dfa21

    
           // headless option for integration test 
        
           if (!options.headless) { 
        
             open(`http://localhost:${port}`) 
        
           }

Type of Pull Request

Adding a feature
Fixing a bug
Maintaining documents
Others (Add test)

Verify the followings

Code is up-to-date with the main branch
No build errors after npm run build
No lint errors after npm run lint
No errors on using charites help globally
Make sure all the exsiting features working well

Refer to CONTRIBUTING.MD for more details.

naogify · 2021-12-09T08:39:12Z

@miya0001 @JinIgarashi

I've added integration tests, could you please review them?

JinIgarashi

Thanks for the PR. I commented two points.

JinIgarashi · 2021-12-09T09:32:43Z

package.json

@@ -9,7 +9,7 @@
    "build": "tsc -p .",
    "watch": "tsc -w",
    "lint": "eslint --fix .",
-    "test": "mocha -r ts-node/register test/*.ts",
+    "test": "npm run build && mocha -r ts-node/register test/*.ts",


Can we use ts-node command to test CLI of typescript directly instead of building before every test?

There is a script command in package.json as below.

charites/package.json

Line 14 in c586c70

"command": "./node_modules/.bin/ts-node ./src/cli"

maybe, it requires to change to ./node_modules/.bin/ts-node ./src/cli.tsin package.json

JinIgarashi · 2021-12-09T09:36:16Z

src/cli/serve.ts

@@ -17,10 +17,12 @@ program
    '--mapbox-access-token [mapboxAccessToken]',
    'Access Token for the Mapbox',
  )
+  .option('--headless', 'serve your map without opening a browser')


Can we use the similar name of option with web pack?

Webpack has --open and --no-open option to avoid opening in browser. I think headless is not intuitive name for users.

https://webpack.js.org/configuration/dev-server/

naogify · 2021-12-17T01:45:18Z

@JinIgarashi

Thank you! I changed the option name to --no-open.

I also tried ts-node, but then each test failed with a timeout.I know that if I set the timeout to a longer number of seconds, the tests won't fail, but I think it works fine if I run npm run build first.

If there is any other advantage to using ts-node, can you share it with me?

JinIgarashi

@naogify テストするたびに毎回ビルドしてdist以下を作らないといけないというのは微妙ですが、コマンドラインのテストをするという方が重要なので、今のままで良いです。
ちなみに私の環境で今の状態でテストをした場合、CLIのテスト部分でタイムアウトしてしまいます（ts-nodeを使っていなくても）。

JinIgarashi · 2021-12-17T08:28:19Z

src/cli/serve.ts

@@ -17,12 +17,12 @@ program
    '--mapbox-access-token [mapboxAccessToken]',
    'Access Token for the Mapbox',
  )
-  .option('--headless', 'serve your map without opening a browser')
+  .option('--no-open', 'serve your map without opening a browser')


--no-openというオプション名なのに、実際の変数名や挙動はopenと正反対のものになっています。個人的には--openというオプションをつけたらブラウザを起動するという動きにした方がわかりやすい気がします。デフォルトではブラウザは起動しない方が良いかと。Webpackでも他のツールでも、デフォルトではブラウザを起動しないというのが普通な気がするので特に違和感ないと思います。

--no-openというオプション名を使うのであれば、それに合わせた変数名と実装に修正をお願いします。

@JinIgarashi

フィードバックありがとうございます。

--no-openというオプション名なのに、実際の変数名や挙動はopenと正反対のものになっています。

こちらは commander js の仕様がそうなっているので、そうしています。
https://github.com/tj/commander.js/#other-option-types-negatable-boolean-and-booleanvalue

デフォルトではブラウザは起動しない方が良いかと。Webpackでも他のツールでも、デフォルトではブラウザを起動しないというのが普通な気がするので特に違和感ないと思います。

スタイルの変更をブラウザでプレビューする以外の用途で、charites serve を実行するユースケースは思いつかないので、大半のユーザーにとってはデフォルトでブラウザを開く方が親切かと思うのですがどうでしょうか？

それと Webpackや他のツールでのデフォルトがブラウザを起動しないから、charites のデフォルトもそうすると言うのは理由になり得にくいと思いました。ツールごとにユースケースが異なるのでその目的に合わせて考える方が良いと思います。

naogify · 2021-12-17T13:36:20Z

@JinIgarashi

ちなみに私の環境で今の状態でテストをした場合、CLIのテスト部分でタイムアウトしてしまいます（ts-nodeを使っていなくても）。

こちらタイムアウトの時間を 10秒に設定しました。五十嵐さんの環境でタイムアウトしないかご確認お願いしてもよろしいでしょうか？

JinIgarashi · 2021-12-17T16:11:30Z

@naogify
まだタイムアウトしてしまいますね。なぜだろうか。なんかテストの実装方法に問題がありそう。
これが解決できればマージします。

$node -v
v16.13.0
$npm -v
8.1.0

$npm test

> @unvt/charites@0.1.2 test
> npm run build && mocha -r ts-node/register test/*.ts --timeout 10000


> @unvt/charites@0.1.2 build
> tsc -p .



  Test for the `build-sprite.ts`.
    ✓ should create icon's json/png at specified path and name 

  Test for the `build.ts`.
    ✓ Should convert `data/style.yml` to JSON.
    ✓ Should minify `data/style.yml` to JSON.
    ✓ Should build command replace sprite url with the option `--sprite-url`.
    ✓ Should build icons with the option `--sprite-url`. Icon file name is set by spriteUrl
    ✓ Use sprite name that specified in style.json with no --sprite-url option
    ✓ Should not create sprite when input directory is not exist.

  Test for the `charites build`
    ✓ charites build style.yml (837ms)
    ✓ charites build style.yml custom.json (925ms)
    ✓ charites build style.yml --compact-output (1121ms)
    ✓ charites build style.yml style.json --sprite-url http://localhost:8080/icons (752ms)
    ✓ charites build style.yml style.json --sprite-input icons --sprite-output . (756ms)

  Test for the `charites convert`
    ✓ charites convert style.json style.yml (815ms)

  Test for the `charites convert`
    ✓ charites init style.yml --tilejson-urls ${tileJsonUrl} (1122ms)
    ✓ charites init style.yml --metadatajson-urls ${metadataJsonUrl} (1069ms)
    ✓ charites init style.yml --tilejson-urls ${tileJsonUrl} --composite-layers (872ms)

  Test for the `charites serve`
    1) charites serve style.yml
    2) charites serve style.yml --provider mapbox --mapbox-access-token xxxxxxx

  Test for the `convert.ts`.
    ✓ Should convert `data/convert.json` to YAML.
    ✓ Should create layers directory.

  Test for the `get-sprite-slug.ts`.
    ✓ should get sprite slug from style.json
    ✓ should return false with no icon's slug in url

  Test for the `init.ts`.
    ✓ Should initialize default style.yml.
    ✓ Should initialize default composited style.yml from tilejson provided (81ms)
    ✓ Should produce composited style.yml without layers if tilejson without vector_layers is specified (98ms)
    ✓ Should initialize default decomposited style.yml from tilejson provided (59ms)
    ✓ Should initialize default decomposited style.yml from metadatajson provided (88ms)

  Test for the `validate-style.ts`.
    ✓ should validate as expected.

  Test for the `yaml-parser.ts`.
    ✓ should parse `data/example.yml`.


  27 passing (29s)
  2 failing

  1) Test for the `charites serve`
       charites serve style.yml:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/j_igarashi/Documents/git/unvt/charites/test/command.serve.spec.ts)
      at listOnTimeout (node:internal/timers:557:17)
      at processTimers (node:internal/timers:500:7)

  2) Test for the `charites serve`
       charites serve style.yml --provider mapbox --mapbox-access-token xxxxxxx:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/j_igarashi/Documents/git/unvt/charites/test/command.serve.spec.ts)
      at listOnTimeout (node:internal/timers:557:17)
      at processTimers (node:internal/timers:500:7)

…add-bdd-test

naogify · 2022-01-05T02:45:53Z

@JinIgarashi

serve 用のテスト内で setTimeout を使っているので、待ち時間を 1秒変更してみました。
これで試してもらっても良いでしょうか？

JinIgarashi · 2022-01-05T08:49:24Z

@naogify ダメですね。serveのテストでまだ落ちます。

  Test for the `charites serve`
    1) charites serve style.yml
    2) charites serve style.yml --provider mapbox --mapbox-access-token xxxxxxx

  27 passing (40s)
  2 failing

  1) Test for the `charites serve`
       charites serve style.yml:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/j_igarashi/Documents/git/unvt/charites/test/command.serve.spec.ts)
      at listOnTimeout (node:internal/timers:557:17)
      at processTimers (node:internal/timers:500:7)

  2) Test for the `charites serve`
       charites serve style.yml --provider mapbox --mapbox-access-token xxxxxxx:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/j_igarashi/Documents/git/unvt/charites/test/command.serve.spec.ts)
      at listOnTimeout (node:internal/timers:557:17)
      at processTimers (node:internal/timers:500:7)

JinIgarashi · 2022-01-05T08:51:50Z

@naogify あとmainブランチから最新をマージして、serveに追加いただいたオプションをドキュメントに反映もお願いします。

以下のページが該当箇所と思います。ページの右上のEdit on Githubから編集対象のソースコードに飛べます。
https://unvt.github.io/charites/usage/commandline_interface.html#realtime-editor-on-browser

naogify · 2022-01-05T08:52:34Z

@JinIgarashi
これ何でですかね？思い当たる原因あったりしますか？
自分の方では、node と npm のバージョンを揃えても再現しませんでした。

…test

naogify · 2022-01-05T08:58:44Z

@JinIgarashi
追加しました!

@naogify あとmainブランチから最新をマージして、serveに追加いただいたオプションをドキュメントに反映もお願いします。

以下のページが該当箇所と思います。ページの右上のEdit on Githubから編集対象のソースコードに飛べます。 https://unvt.github.io/charites/usage/commandline_interface.html#realtime-editor-on-browser

JinIgarashi · 2022-01-05T09:10:22Z

これ何でですかね？思い当たる原因あったりしますか？
自分の方では、node と npm のバージョンを揃えても再現しませんでした。

@naogify 何が原因か自分にはわかりません。

naogify · 2022-01-05T09:15:23Z

@JinIgarashi
自分の方では再現出来ず、調査ができないので五十嵐さんの環境で調査をお願いしても良いでしょうか？

JinIgarashi · 2022-01-05T09:50:51Z

@naogify 一旦serveだけ外してマージしてもいいかも。その上で、serveコマンドだけテストのやり方を見直してみたらどうでしょうか？
webpackとか他のブラウザ開くライブラリがどうテストしてるかみてみると参考になるかも。。。テストが実装しやすいソースコードに変更する必要もあるかもしれないですね
https://github.com/webpack/webpack-dev-server/tree/master/test/server

JinIgarashi · 2022-01-05T10:32:24Z

@naogify 今のserveコマンドみたいに一つの関数の中で全てを行うような実装の仕方だとテストを書くにくいコードになってるけれども、最低限サーバー起動部分とそうでない部分を切り離したソースコードにした上で、ユニットテストを書くといいかもしれません。下の記事はそんな感じになってる。

https://medium.com/@basavarajkn/integration-testing-websocket-server-in-node-js-2997d107414c
http://codereform.com/blog/post/socket-io-integration-tests-with-chai-and-mocha/

大体どの記事もユニットテストの中で、専用のサーバー立ち上げてテストみたいな動きになってますね。

naogify · 2022-02-13T13:33:52Z

@JinIgarashi
この件、遅くなっていてすみません! サンプルで頂いたコードを参考にして修正してみます。

…add-bdd-test

naogify · 2022-03-10T04:55:37Z

@hfu @ubukawa @JinIgarashi

I added integrated tests for commands below. What do you think?

charites init.
charites build.
charites convert.

Adding tests for charites serve and charites build --watch takes time for me, so I went ahead and implemented them with the above commands.

I would be happy to review it.

JinIgarashi

Looks good. Thanks

naogify · 2022-03-18T05:58:27Z

@JinIgarashi
Sorry I can't merge my self. Could you merge it? or approve it again ? Thank you!

JinIgarashi · 2022-03-18T21:27:48Z

@naogify I have approved again. However I can’t merge this PR now. I think I have no longer been permitted with the write access for charites.

add integration test

c586c70

naogify force-pushed the add-bdd-test branch from 0e61a00 to c586c70 Compare December 9, 2021 08:33

naogify requested review from miya0001 and JinIgarashi December 9, 2021 08:34

JinIgarashi reviewed Dec 9, 2021

View reviewed changes

JinIgarashi linked an issue Dec 9, 2021 that may be closed by this pull request

Add unit test for serve command #62

Closed

change option name

8fa9ed0

JinIgarashi suggested changes Dec 17, 2021

View reviewed changes

set test timeout 10000

144a1e0

naogify added 2 commits January 5, 2022 11:41

set 1000 for serve test

6517721

Merge branch 'add-bdd-test' of https://github.com/unvt/charites into …

c4b6725

…add-bdd-test

naogify added 2 commits January 5, 2022 17:54

Merge branch 'main' of https://github.com/unvt/charites into add-bdd-…

f19715e

…test

add --no-open to documentation

5936885

naogify added 3 commits March 10, 2022 13:22

remove test for serve

4ea6c8d

Merge branch 'main' into add-bdd-test

ad0868f

fix test script

63974e5

naogify added 5 commits March 10, 2022 13:29

Merge branch 'add-bdd-test' of https://github.com/unvt/charites into …

c07d973

…add-bdd-test

remove unused module

b941602

some fix

37ba6fd

some fix

810fb27

some fix

118d64a

naogify requested review from hfu and ubukawa March 10, 2022 05:02

JinIgarashi approved these changes Mar 10, 2022

View reviewed changes

naogify removed request for hfu, miya0001 and ubukawa March 18, 2022 05:54

JinIgarashi approved these changes Mar 18, 2022

View reviewed changes

hfu approved these changes Mar 18, 2022

View reviewed changes

hfu merged commit edc0aa9 into main Mar 18, 2022

hfu deleted the add-bdd-test branch March 18, 2022 23:57

JinIgarashi mentioned this pull request Mar 19, 2022

Add unit test for serve command #62

Closed

keichan34 mentioned this pull request Dec 19, 2022

Support for sprites on serve #143

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BDD test #80

Add BDD test #80

naogify commented Dec 9, 2021

naogify commented Dec 9, 2021

JinIgarashi left a comment

JinIgarashi Dec 9, 2021

JinIgarashi Dec 9, 2021

JinIgarashi Dec 9, 2021

naogify commented Dec 17, 2021

JinIgarashi left a comment

JinIgarashi Dec 17, 2021

naogify Dec 17, 2021 •

edited

Loading

naogify commented Dec 17, 2021

JinIgarashi commented Dec 17, 2021

naogify commented Jan 5, 2022 •

edited

Loading

JinIgarashi commented Jan 5, 2022

JinIgarashi commented Jan 5, 2022

naogify commented Jan 5, 2022 •

edited

Loading

naogify commented Jan 5, 2022

JinIgarashi commented Jan 5, 2022 •

edited

Loading

naogify commented Jan 5, 2022 •

edited

Loading

JinIgarashi commented Jan 5, 2022 •

edited

Loading

JinIgarashi commented Jan 5, 2022

naogify commented Feb 13, 2022

naogify commented Mar 10, 2022

JinIgarashi left a comment

naogify commented Mar 18, 2022

JinIgarashi commented Mar 18, 2022 •

edited

Loading

	// headless option for integration test
	if (!options.headless) {
	open(`http://localhost:${port}`)
	}

Add BDD test #80

Add BDD test #80

Conversation

naogify commented Dec 9, 2021

Description

Type of Pull Request

Verify the followings

naogify commented Dec 9, 2021

JinIgarashi left a comment

Choose a reason for hiding this comment

JinIgarashi Dec 9, 2021

Choose a reason for hiding this comment

JinIgarashi Dec 9, 2021

Choose a reason for hiding this comment

JinIgarashi Dec 9, 2021

Choose a reason for hiding this comment

naogify commented Dec 17, 2021

JinIgarashi left a comment

Choose a reason for hiding this comment

JinIgarashi Dec 17, 2021

Choose a reason for hiding this comment

naogify Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

naogify commented Dec 17, 2021

JinIgarashi commented Dec 17, 2021

naogify commented Jan 5, 2022 • edited Loading

JinIgarashi commented Jan 5, 2022

JinIgarashi commented Jan 5, 2022

naogify commented Jan 5, 2022 • edited Loading

naogify commented Jan 5, 2022

JinIgarashi commented Jan 5, 2022 • edited Loading

naogify commented Jan 5, 2022 • edited Loading

JinIgarashi commented Jan 5, 2022 • edited Loading

JinIgarashi commented Jan 5, 2022

naogify commented Feb 13, 2022

naogify commented Mar 10, 2022

JinIgarashi left a comment

Choose a reason for hiding this comment

naogify commented Mar 18, 2022

JinIgarashi commented Mar 18, 2022 • edited Loading

naogify Dec 17, 2021 •

edited

Loading

naogify commented Jan 5, 2022 •

edited

Loading

naogify commented Jan 5, 2022 •

edited

Loading

JinIgarashi commented Jan 5, 2022 •

edited

Loading

naogify commented Jan 5, 2022 •

edited

Loading

JinIgarashi commented Jan 5, 2022 •

edited

Loading

JinIgarashi commented Mar 18, 2022 •

edited

Loading