nmanhit commented 1 year ago

Regarding coverage, some commands don't reach the coverage goal, but they passed it.

It may be the difference of the coverageThreshold's parameter. Here is KUC. Please double-check it. And if possible, please improve for Auth and Dev commands. https://github.com/kintone-labs/kintone-ui-component/blob/3c2e25970ac5b0126dc3c7954584320a5e1c5ff2/web-test-runner.config.mjs#L10

There are some unnecessary import sentences, so please remove them. For example, here.

https://github.com/kintone-labs/kintone-cli/blob/c79b9b52c92ee0c9a1443f74f5ae448db6372749/src/commands/Dev/test/uploadConfig.test.ts#L7

https://github.com/kintone-labs/kintone-cli/blob/c79b9b52c92ee0c9a1443f74f5ae448db6372749/src/commands/Dev/test/uploadConfig.test.ts#L12

There are some commands that have validator test cases and do not have them. Please unify it.

We have edited the Jest configuration here https://github.com/kintone-labs/kintone-cli/pull/112/files#diff-1e058ca1442e46581b13571fb8d261f9e1f5657e26c96634d4c1072f0f0347f1R10-R54 When I read the Jest documentation, I thought that configuring it as
```
'./src/commands/**': {
  statements: 85,
  branches: 85,
  functions: 85,
  lines: 85
},
```
ref: https://jestjs.io/docs/configuration#coveragethreshold-object However, it doesn't work with that configuration.
we have removed https://github.com/kintone-labs/kintone-cli/pull/112/commits/dca2c0759e43f67a11fcb4eb06d2b728177a58eb

TomokoMiyake commented 1 year ago

We have edited the Jest configuration here https://github.com/kintone-labs/kintone-cli/pull/112/files#diff-1e058ca1442e46581b13571fb8d261f9e1f5657e26c96634d4c1072f0f0347f1R10-R54

Thank you for updating!

But still, there are some commands that don't reach the goal even though the test command passed. Please double check it. スクリーンショット 2023-08-22 16 58 53

we have removed https://github.com/kintone-labs/kintone-cli/commit/dca2c0759e43f67a11fcb4eb06d2b728177a58eb

I thought the link isn't relevant to the theme. But I confirmed that there are no unnecessary import lines anymore.

There are some commands that have validator test cases and do not have them. Please unify it.

How about this?

hoangtran2506 commented 1 year ago

There are some commands that have validator test cases and do not have them. Please unify it.

How about this?

I changed the "validator" file name to the name that is suitable for test cases. I updated the solutions in here: https://github.com/kintone-labs/kintone-cli/pull/112/files/640895fb1f0b2fdb9eb0cc4a8148252b6ce5db6d..01992b87833e7d379cd354d15fb8e02012f676cd

Please have a look! Thank you!

TomokoMiyake commented 1 year ago

I changed the "validator" file name to the name that is suitable for test cases. I updated the solutions in here: https://github.com/kintone-labs/kintone-cli/pull/112/files/640895fb1f0b2fdb9eb0cc4a8148252b6ce5db6d..01992b87833e7d379cd354d15fb8e02012f676cd

Please have a look! Thank you!

Sorry, could you please tell me the exact files you changed for this? The above file changes might include other than the validator-related ones.

hoangtran2506 commented 1 year ago

Sorry, could you please tell me the exact files you changed for this? The above file changes might include other than the validator-related ones.

https://github.com/kintone-labs/kintone-cli/pull/112/commits/e1ca101f7e0bb02253afdbbb8d73c55ec4db65bb#diff-4bde85001c75455173dbbafd3b350ba57dd5679a51a0b28bda204a35ebdb7a40

https://github.com/kintone-labs/kintone-cli/pull/112/commits/52c99839f7d87a67c5ba9b082b68abb38b93c0bc

The detail is: Initialize/test/validator.test.ts is divided into:

Initialize/test/scope.test.ts
Initialize/test/preset.test.ts
Add code to Initialize/test/appType.test.ts

Lint/test/validator.test.ts is changed to:

Lint/test/fixOption.test.ts

TomokoMiyake commented 1 year ago

validator test cases

The detail is: Initialize/test/validator.test.ts is divided into:

Initialize/test/scope.test.ts

Initialize/test/preset.test.ts

Add code to Initialize/test/appType.test.ts

In the preset.test.ts file, there is only an invalid preset test case. Don't we need a valid preset test case?

Lint/test/validator.test.ts is changed to:

Lint/test/fixOption.test.ts

Ok!

For other commands... It seems there is no test case for Build/validator.ts, Deploy/validator.ts, Dev_validator.ts. Please check it.

Coverage

About Dev command, the coverage is low. Could you please double-check it. If it's difficult to improve it, please tell me.

And I'm confused that the test passed even though the coverage doesn't meet the goal. Why is this happening?

Error

When executing test, this two errors occurred. Will the same error occur on your environment?

About this Japanese part, it is "Error occurred. Failed to login to kintone. Please check the base URL." エラーが発生しました kintoneへのログインに失敗しました。ベースURLを確認してください。

hoangtran2506 commented 1 year ago

In the preset.test.ts file, there is only an invalid preset test case. Don't we need a valid preset test case?

We don't need a valid preset test because:

When passing a valid preset, the action next is to test for the scope case
Don't have code for valid preset

For other commands... It seems there is no test case for Build/validator.ts, Deploy/validator.ts, Dev_validator.ts. Please check it.

The test case for validators in commands is placed in the right contexts, for example:

In validator of Build has function that checks the app name
So we put the test case to appName.test.ts
Similar to another command
The reason for changing this behavior is:
- When the validator file has too many cases for testing, the validator test file will have too much code for testing, this is hard to read, maintain, and improve. I think we discussed it here: https://bozuman.cybozu.com/k/23175/show#record=3283&comment=72

About Dev command, the coverage is low.

Currently, I can't find a way to improve it, it is stuck with --watch, and other javascript events. Sorry for the inconvenience.

And I'm confused that the test passed even though the coverage doesn't meet the goal. Why is this happening?

The coverage of Dev command does not pass, this has little effect on the whole coverage

But if we have two or three commands that do not pass, the coverage will slow.

The test passed because the total coverage is more than 85%

When executing test, this two errors occurred. Will the same error occur on your environment?

Because we test on the local environment, we can't log in with it. It will have some errors when the code has actions that interact with the kintone environment

TomokoMiyake commented 1 year ago

We don't need a valid preset test because:

When passing a valid preset, the action next is to test for the scope case

Don't have code for valid preset

I got it! If so, there's no problem with it. Thanks for the confirmation.

The test case for validators in commands is placed in the right contexts, for example:

In validator of Build has function that checks the app name

So we put the test case to appName.test.ts

Similar to another command

The reason for changing this behavior is:

When the validator file has too many cases for testing, the validator test file will have too much code for testing, this is hard to read, maintain, and improve. I think we discussed it here: https://bozuman.cybozu.com/k/23175/show#record=3283&comment=72

Yes, I understand the discussion. I know you basically developed validator-related function test cases in each separate option test file. But I couldn't find the exact test case for Build/validator.ts, Deploy/validator.ts, Dev_validator.ts functions. For example, when there is no appName, program will return 'App name missing', but there is no test case for it. I found this because you developed a similar test case here. https://github.com/kintone-labs/kintone-cli/commit/52c99839f7d87a67c5ba9b082b68abb38b93c0bc#diff-e0073e61c60ec0ba6020821f942dcabe5b6f151e029ec3509649df190576f541

The coverage of Dev command does not pass, this has little effect on the whole coverage

But if we have two or three commands that do not pass, the coverage will slow.

The test passed because the total coverage is more than 85%

That's why! Sorry for my lack of knowledge. I understand it now and it's ok.

Because we test on the local environment, we can't log in with it. It will have some errors when the code has actions that interact with the kintone environment

I see... When executing on CI, will it work fine with no error because it has an environment variable for login??

hoangtran2506 commented 1 year ago

Yes, I understand the discussion. I know you basically developed validator-related function test cases in each separate option test file. But I couldn't find the exact test case for Build/validator.ts, Deploy/validator.ts, Dev_validator.ts functions. For example, when there is no appName, program will return 'App name missing', but there is no test case for it. I found this because you developed a similar test case here.

With Buid, Deploy, Dev commands, I write test cases for functions in the validator by calling the main function In that main function, there are call functions of validator: validator.devValidator(cmd), with that, the jest will cover the code of functions in the validator. For example with dev command:

I think it makes sense for the validator of each command, if you want to add test for validator like the Lint command, I will update the code

I will list the test code for the validator of each command below:

I see... When executing on CI, will it work fine with no error because it has an environment variable for login??

It will have errors when executing on CI, but it does not affect CI because our jest does not handle that case.

Our jest just tests for the logic of code and doesn't support testing for interaction with the kintone enviroment
The domain for test is example domain "https://domain.kintone.com", it doesn't exist.

You can see detail in here: https://github.com/kintone-labs/kintone-cli/actions/runs/5971290256/job/16200124838

TomokoMiyake commented 1 year ago

I think it makes sense for the validator of each command, if you want to add test for validator like the Lint command, I will update the code

Thanks for understanding my thoughts. You have already achieved the coverage, so it's not a necessary item, but it would be great if you could add the test cases because we recognize it now!

It will have errors when executing on CI, but it does not affect CI because our jest does not handle that case.

I got it. Let's skip the error output because it's not directly related to the testing behavior.

hoangtran2506 commented 1 year ago

Thanks for understanding my thoughts. You have already achieved the coverage, so it's not a necessary item, but it would be great if you could add the test cases because we recognize it now!

I updated code for the validator here: https://github.com/kintone-labs/kintone-cli/pull/112/commits/5c5c805f31466ac8ccfdd9e57b8e747728ae4d52

kintone-labs / kintone-cli

SSR-3283 Create a unit test project #112

validator test cases

Coverage

Error