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

fix: add better failover case for duration not being found #17

Merged
merged 3 commits into from Jul 13, 2022
Merged

fix: add better failover case for duration not being found #17

merged 3 commits into from Jul 13, 2022

Conversation

BobGneu
Copy link
Contributor

@BobGneu BobGneu commented Jul 13, 2022

This past week I have been on a tirade of skipping test cases, and I found that there was a case where if a suite is skipped the duration could be undefined, given there is either no value or no result for it.

I have included the failover of reporting the file, which I am on the fence about, but more importantly, both paths through the method to retrieve the duration return a duration.

Happy to discuss or update further!

if (typeof duration !== 'number') {
return '';
if (typeof duration !== "number") {
return ` duration="0"`;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems correct. 👍
Duration is a mandatory field based on SonarQube documentation.

Inside a file element, insert a testCase for each test run by unit tests. It can have the following attributes/children:

  • testCase (mandatory)

    • duration (mandatory): long value in milliseconds

src/xml.ts Outdated Show resolved Hide resolved
BobGneu and others added 2 commits July 13, 2022 00:35
Co-authored-by: Ari Perkkiö <ari.perkkio@gmail.com>
Copy link
Owner

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @BobGneu!

@AriPerkkio AriPerkkio merged commit 452ab59 into AriPerkkio:main Jul 13, 2022
@AriPerkkio
Copy link
Owner

This is now released in version 0.3.1.

@BobGneu
Copy link
Contributor Author

BobGneu commented Jul 13, 2022

I went to update the snapshots and wanted to note, running the tests requires yarn, which could be replaced with the following with no other impact on the suite, beyond the absolute paths being present.

    try {
        execSync('npx vitest --config test/vite.test-config.ts', {
            stdio: 'inherit',
        });
    } catch (_) {
        // Ignore exit codes
    }

Could lead to better engagement with the community as vitest continues to gain a foothold.
Either way, thank you for the plugin, and your contributions to the community =)

@AriPerkkio
Copy link
Owner

Good catch! I definitely want to improve the contribution experience and lower the requirements.

It seems that we can even reference vitest binary directly, since these tests are run by using package.json's scripts. The node_modules/.bin binaries are available in the session.
This will be resolved in #18.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants