Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete tmp directory from coverage report after running #2108

Closed
4 tasks done
kaioduarte opened this issue Oct 1, 2022 · 9 comments · Fixed by #2144
Closed
4 tasks done

Delete tmp directory from coverage report after running #2108

kaioduarte opened this issue Oct 1, 2022 · 9 comments · Fixed by #2144
Labels
feat: coverage Issues and PRs related to the coverage feature

Comments

@kaioduarte
Copy link

Clear and concise description of the problem

The /coverage/tmp directory is preserved after the code coverage report is finished. That's fine for local development, but it's not on CI when we need to publish the results to an analysis tool like CodeClimate.

For example, this is the size of the temp directory for a project I'm working on:

435M	coverage/tmp
4.9M	coverage/xxx-api
1.7M	coverage/coverage-final.json
988K	coverage/clover.xml
 84K	coverage/index.html
 20K	coverage/prettify.js
8.0K	coverage/sorter.js
8.0K	coverage/base.css
4.0K	coverage/sort-arrow-sprite.png
4.0K	coverage/prettify.css
4.0K	coverage/favicon.png
4.0K	coverage/block-navigation.js

Suggested solution

Add an option to the coverage config to remove the tmp directory.

Alternative

For now, I can remove the files after running the coverage script, like:

  "test:coverage": "vitest run --coverage && rm -rf ./coverage/tmp"

Additional context

No response

Validations

@sheremet-va sheremet-va added bug feat: coverage Issues and PRs related to the coverage feature labels Oct 1, 2022
@kaioduarte
Copy link
Author

@sheremet-va I'd like to work on this feature if you think it's worth adding a new config :)

@sheremet-va
Copy link
Member

I think it's a bug. We have code for deleting coverage folder.

@kaioduarte
Copy link
Author

Do you mean to delete the entire coverage folder on rerun?

I saw this line:

await this.coverageProvider?.clean(this.config.coverage.clean)

But what I'm looking for is to delete only the tmp directory.

@AriPerkkio
Copy link
Member

I think I've looked into this before. Even if you tell v8 to stop coverage collection with require('v8').stopCoverage(), it will still output the final v8 coverage reports into directory pointed by process.env.NODE_V8_COVERAGE when vitest's main process exits. Simply running fs.rm for the tmp directory is not enough.

@kaioduarte
Copy link
Author

@AriPerkkio, I didn't notice that behavior, but it's still worth deleting the tmp dir. In my case, it created a new 1MB file, and it's way better than the current behavior.

@AriPerkkio
Copy link
Member

Sure, I'd like it to work that way. If possible, the tmp directory should always be removed after coverage report is generated. Users should not be interested in pure v8 reports.

It might be worth looking into whether we even need to set process.env.NODE_V8_COVERAGE in the main process. After #1278 was implemented I think we only need to set the flag in workers.

@everett1992
Copy link
Contributor

Would it be possible to remove the whole coverage directory if it only includes tmp, for instance when using a console text reporter?

@AriPerkkio
Copy link
Member

AriPerkkio commented Oct 12, 2022

@kaioduarte #2144 will reduce the size of leftover coverage/tmp reports. I think that is the best we can do at the moment.

@everett1992 I don't think it's possible. It seems that vitest has to set NODE_V8_COVERAGE in its main process in order to get coverage reported. I did some testing and it looks like V8 coverage reporting cannot be enabled in worker_threads (used by Tinypool) directly. This is the only comment I found about this issue nodejs/node#28283 (comment).

We could remove the coverage report here outside vitest process, but adding coverage logic here is not ideal.

const { stderr = '' } = await child

The "leftover" v8 reports are written when this process exits:

if (await startVitest(mode, cliFilters, options) === false)
process.exit()

Manual testing to see whether NODE_V8_COVERAGE works in worker_threads
> node run.js
#1 existsSync(NODE_V8_COVERAGE) false

# Run in child_process
script::NODE_V8_COVERAGE /workspaces/example-project/temp-coverage
#2 existsSync(NODE_V8_COVERAGE) true

Removing /workspaces/example-project/temp-coverage
#3 existsSync(NODE_V8_COVERAGE) false

# Run in worker_thread
script::NODE_V8_COVERAGE /workspaces/example-project/temp-coverage
#4 existsSync(NODE_V8_COVERAGE) false
// script.js
console.log('script::NODE_V8_COVERAGE', process.env.NODE_V8_COVERAGE);
// run.js
const { execFileSync } = require("child_process");
const { Worker } = require("worker_threads");
const { existsSync, rmSync } = require("fs");

const NODE_V8_COVERAGE = "/workspaces/example-project/temp-coverage";

if (existsSync(NODE_V8_COVERAGE)) {
    rmSync(NODE_V8_COVERAGE, { recursive: true, force: true })
}

checkDirectory('#1')

// This will enable V8 coverage report
execFileSync("/usr/local/bin/node", ["script.js"], {
    stdio: 'inherit',
    env: { NODE_V8_COVERAGE }
});

checkDirectory('#2')

if (existsSync(NODE_V8_COVERAGE)) {
    console.log('Removing', NODE_V8_COVERAGE)
    rmSync(NODE_V8_COVERAGE, { recursive: true, force: true })
}

checkDirectory('#3')

// This will NOT enable V8 coverage report
const worker = new Worker('./script.js', { env: { NODE_V8_COVERAGE } })

new Promise((resolve, reject) => {
    worker.on('exit', resolve);
    worker.on('error', reject);
}).then(() => {
    checkDirectory('#4')
});


// Helpers
function checkDirectory(label) {
    console.log(label, 'existsSync(NODE_V8_COVERAGE)', existsSync(NODE_V8_COVERAGE))
}

@kaioduarte
Copy link
Author

kaioduarte commented Oct 13, 2022

Thanks for investigating, @AriPerkkio!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feat: coverage Issues and PRs related to the coverage feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants