feat: Add code coverage support

[TamiTakamiya]

(Continue from #1227)

Before submitting your PR, please review the following checklist:

• CONSIDER adding a new test if your PR resolves an issue.
• DO keep pull requests small so they can be easily reviewed.
• DO make sure tests pass.
• DO make sure any public APIs changes are documented.
• DO make sure not to introduce any compiler warnings.

Before merging the PR:

• CHECK continuous integration of main branch is green.
• CHECK pull request check job is green.
• CHECK all pull request questions/requests are resolved.
• WAIT till PR is approved by at least 1 committer.

[TamiTakamiya]

@djelinek I think this PR is now ready for review. Let me know if you have further comments. Thanks!

[TamiTakamiya]

@djelinek Though it is not directly related to code coverage support, is it OK to add supports for MOCHA_GREP & MOCHA_INVERT environment variable support? It is these 6 lines found in electron's repo.

With the support, we can specify a filter of Mocha test cases to be executed. For example, it can be used with the coverage support like:

MOCHA_GREP='Single WebView' npm run coverage

If I'd better open a separate issue/PR, I will do that accordingly.

[djelinek]

@djelinek Though it is not directly related to code coverage support, is it OK to add supports for MOCHA_GREP & MOCHA_INVERT environment variable support? It is these 6 lines found in electron's repo.

With the support, we can specify a filter of Mocha test cases to be executed. For example, it can be used with the coverage support like:

MOCHA_GREP='Single WebView' npm run coverage

If I'd better open a separate issue/PR, I will do that accordingly.

is that something different then using .mochaarc config file? I think you can achieve mentioned eg for grep there is a example https://github.com/mochajs/mocha/blob/master/example/config/.mocharc.yml#L24

I am asking because ExTester is currently supporting this configs

[TamiTakamiya]

is that something different then using .mochaarc config file? I think you can achieve mentioned eg for grep there is a example https://github.com/mochajs/mocha/blob/master/example/config/.mocharc.yml#L24

I am asking because ExTester is currently supporting this configs

Yes, it works in the same way. The difference is in that user can filter the test cases to be executed without changing config files. I found it useful in develiopment phase.

[TamiTakamiya]

is that something different then using .mochaarc config file? I think you can achieve mentioned eg for grep there is a example https://github.com/mochajs/mocha/blob/master/example/config/.mocharc.yml#L24
I am asking because ExTester is currently supporting this configs

Yes, it works in the same way. The difference is in that user can filter the test cases to be executed without changing config files. I found it useful in develiopment phase.

I have added following lines to runner.ts:

        if (process.env.MOCHA_GREP) {
            conf.grep = process.env.MOCHA_GREP;
        }
        if (process.env.MOCHA_INVERT) {
            conf.invert = process.env.MOCHA_INVERT === 'true';
        }

[TamiTakamiya]

@djelinek I believe this is prepared for another examination. Could you please review it once more? Thanks!

[djelinek]

@djelinek I believe this is prepared for another examination. Could you please review it once more? Thanks!

yes, i am sorry, I will try and get back ASAP

[djelinek]

is that something different then using .mochaarc config file? I think you can achieve mentioned eg for grep there is a example https://github.com/mochajs/mocha/blob/master/example/config/.mocharc.yml#L24
I am asking because ExTester is currently supporting this configs

Yes, it works in the same way. The difference is in that user can filter the test cases to be executed without changing config files. I found it useful in develiopment phase.

I have added following lines to runner.ts:

        if (process.env.MOCHA_GREP) {
            conf.grep = process.env.MOCHA_GREP;
        }
        if (process.env.MOCHA_INVERT) {
            conf.invert = process.env.MOCHA_INVERT === 'true';
        }

that is cool, but I think it would be better to provide in separate PR.. could you please move it to another one?

[TamiTakamiya]

@djelinek OK I will create a separate PR for MOCHA_GREP/MOCHA_INVERT.

On a different topic... I noticed two test cases failed in npm run test:coverage. When the coverage option is ON, .vsix file is not installed and codes are loaded from the development directory, but these two test cases assumes the test extension is installed from a .vsix file. I think there are a few options to resolve this:

1. Install .vsix file even when the coverage option is ON. This is the easiest one, but I am not sure if it is a good idea to install it only for fixing those test failures and the installed extension is not used in the execution.
2. Skip these test cases when the coverage option is ON. This is a cleaner approach but those cases won't be covered in code coverage.
3. Install .vsix file explicity before running npm run test:coverage. Currently uninstallation is skipped when the coverage options is ON, but it needs to be enabled to clean up the extension after npm run test:coverage.

What do you think? I think option 1 would be acceptable because currently the .vsix installation is mandatory for running ExTester.

  298 passing (10m)
  11 pending
  2 failing

  1) ExtensionsView
       findItem works:
     Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/ttakamiy/git/redhat-developer/vscode-extension-tester/tests/test-project/out/test/xsideBar/xtensionsView.test.js)
      at listOnTimeout (node:internal/timers:569:17)
      at process.processTimers (node:internal/timers:512:7)

  2) ExtensionsView
       ExtensionsViewItem
         "before each" hook for "getTitle works":
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/ttakamiy/git/redhat-developer/vscode-extension-tester/tests/test-project/out/test/xsideBar/xtensionsView.test.js)
      at listOnTimeout (node:internal/timers:569:17)
      at process.processTimers (node:internal/timers:512:7)

[TamiTakamiya]

#1262 was opened for MOCHA_GREP/MOCHA_INVERT environment variable support.

[TamiTakamiya]

@djelinek OK I will create a separate PR for MOCHA_GREP/MOCHA_INVERT.

On a different topic... I noticed two test cases failed in npm run test:coverage. When the coverage option is ON, .vsix file is not installed and codes are loaded from the development directory, but these two test cases assumes the test extension is installed from a .vsix file. I think there are a few options to resolve this:

1. Install .vsix file even when the coverage option is ON. This is the easiest one, but I am not sure if it is a good idea to install it only for fixing those test failures and the installed extension is not used in the execution.
2. Skip these test cases when the coverage option is ON. This is a cleaner approach but those cases won't be covered in code coverage.
3. Install .vsix file explicity before running npm run test:coverage. Currently uninstallation is skipped when the coverage options is ON, but it needs to be enabled to clean up the extension after npm run test:coverage.

What do you think? I think option 1 would be acceptable because currently the .vsix installation is mandatory for running ExTester.

@djelinek I have implemented the option 1 in the latest commit for fixing the failure. Please let me know if you have different opinion. Thanks!

[djelinek]

@djelinek OK I will create a separate PR for MOCHA_GREP/MOCHA_INVERT.
On a different topic... I noticed two test cases failed in npm run test:coverage. When the coverage option is ON, .vsix file is not installed and codes are loaded from the development directory, but these two test cases assumes the test extension is installed from a .vsix file. I think there are a few options to resolve this:

1. Install .vsix file even when the coverage option is ON. This is the easiest one, but I am not sure if it is a good idea to install it only for fixing those test failures and the installed extension is not used in the execution.
2. Skip these test cases when the coverage option is ON. This is a cleaner approach but those cases won't be covered in code coverage.
3. Install .vsix file explicity before running npm run test:coverage. Currently uninstallation is skipped when the coverage options is ON, but it needs to be enabled to clean up the extension after npm run test:coverage.

What do you think? I think option 1 would be acceptable because currently the .vsix installation is mandatory for running ExTester.

@djelinek I have implemented the option 1 in the latest commit for fixing the failure. Please let me know if you have different opinion. Thanks!

those are valid points and I will need to take a deeper look. Currently I am not sure if that is ok to install vsix into vscode instance and then use source of the same extensions as thing to really test with.. it is becoming a bit confusing at the moment

[TamiTakamiya]

@djelinek OK I will create a separate PR for MOCHA_GREP/MOCHA_INVERT.
On a different topic... I noticed two test cases failed in npm run test:coverage. When the coverage option is ON, .vsix file is not installed and codes are loaded from the development directory, but these two test cases assumes the test extension is installed from a .vsix file. I think there are a few options to resolve this:

1. Install .vsix file even when the coverage option is ON. This is the easiest one, but I am not sure if it is a good idea to install it only for fixing those test failures and the installed extension is not used in the execution.
2. Skip these test cases when the coverage option is ON. This is a cleaner approach but those cases won't be covered in code coverage.
3. Install .vsix file explicity before running npm run test:coverage. Currently uninstallation is skipped when the coverage options is ON, but it needs to be enabled to clean up the extension after npm run test:coverage.

What do you think? I think option 1 would be acceptable because currently the .vsix installation is mandatory for running ExTester.

@djelinek I have implemented the option 1 in the latest commit for fixing the failure. Please let me know if you have different opinion. Thanks!

those are valid points and I will need to take a deeper look. Currently I am not sure if that is ok to install vsix into vscode instance and then use source of the same extensions as thing to really test with.. it is becoming a bit confusing at the moment

Option 4 came to my mind today:

4. When the coverage option is ON, let the -u (--uninstall_extension) option decide whether the .vsix installation is installed or not, i.e. if -u is set, the .vsix file is installed and uninstalled. If not, the .vsix file is not installed nor uninstalled.

This re-use of the -u option may sound a bit tricky, but since these are edge cases and I think it would be OK as long as these exceptional behavior is documented. What do you think?

[djelinek]

@djelinek OK I will create a separate PR for MOCHA_GREP/MOCHA_INVERT.
On a different topic... I noticed two test cases failed in npm run test:coverage. When the coverage option is ON, .vsix file is not installed and codes are loaded from the development directory, but these two test cases assumes the test extension is installed from a .vsix file. I think there are a few options to resolve this:

1. Install .vsix file even when the coverage option is ON. This is the easiest one, but I am not sure if it is a good idea to install it only for fixing those test failures and the installed extension is not used in the execution.
2. Skip these test cases when the coverage option is ON. This is a cleaner approach but those cases won't be covered in code coverage.
3. Install .vsix file explicity before running npm run test:coverage. Currently uninstallation is skipped when the coverage options is ON, but it needs to be enabled to clean up the extension after npm run test:coverage.

What do you think? I think option 1 would be acceptable because currently the .vsix installation is mandatory for running ExTester.

@djelinek I have implemented the option 1 in the latest commit for fixing the failure. Please let me know if you have different opinion. Thanks!

those are valid points and I will need to take a deeper look. Currently I am not sure if that is ok to install vsix into vscode instance and then use source of the same extensions as thing to really test with.. it is becoming a bit confusing at the moment

Option 4 came to my mind today:

4. When the coverage option is ON, let the -u (--uninstall_extension) option decide whether the .vsix installation is installed or not, i.e. if -u is set, the .vsix file is installed and uninstalled. If not, the .vsix file is not installed nor uninstalled.

This re-use of the -u option may sound a bit tricky, but since these are edge cases and I think it would be OK as long as these exceptional behavior is documented. What do you think?

so if I understand correctly.. just let me to rephrase..

if --unninstall_extension and --coverage are set

• the extension will be packed into .vsix and installed
• tests will be running against same extension just only taken from directly sources (...brings a bit of duplicity we mentioned before)
• after tests the .vsix extension will be unninstalled

if only --coverage is set

• no .vsix packing or install
• tests are running directly against extension built from sources
• no unistall

if --coverage is not set at all

• the behaviour should remains same as it is now

am I understand it correctly? feel free to correct me.. I am being already a bit trapped here sorry about that 😵‍💫

[TamiTakamiya]

so if I understand correctly.. just let me to rephrase..

if --unninstall_extension and --coverage are set

• the extension will be packed into .vsix and installed
• tests will be running against same extension just only taken from directly sources (...brings a bit of duplicity we mentioned before)
• after tests the .vsix extension will be unninstalled

if only --coverage is set

• no .vsix packing or install
• tests are running directly against extension built from sources
• no unistall

if --coverage is not set at all

• the behaviour should remains same as it is now

am I understand it correctly? feel free to correct me.. I am being already a bit trapped here sorry about that 😵‍💫

Yes, your understanding is correct.

I have implemented it with this commit. Essentially it is these three lines in extester.ts

        if (!this.code.coverageEnabled || cleanup) {
            await this.installVsix({useYarn});
        }

With that change, I ran

MOCHA_GREP=ExtensionsView npm run test:coverage

and it completed successfully. Then I modified tests/test-project/package.json to remove -u from "ui-test" script and re-ran the same test to see the test failed as expected.

[djelinek]

okay. let's kick off this new feature with option 4) that seems usable..

I think one last thing which is this PR missing is to add new page for our wiki about coverage itself.. and a bit explanation how it works

maybe bring the summary about what we discussed here about how it will work, could you please create into docs/ ?

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[TamiTakamiya]

@djelinek I have created CodeCoverage.md under docs/ and made minor modifications on Home.md and Test-Setup.md. As I am not a native English speaker, please update them if you find any expressions that do not sound natural. Thanks!

While doing the updates, I found some new updates (e.g. -r --open_resource option) were not included in Test-Setup.md. The PR contains only the changes for code coverage. We wlll need a separate PR to address those issues.