fix: fail when min_coverage is not a number

[kelvinwieth]

Description

Solves #255

Type of Change

• ✨ New feature (non-breaking change which adds functionality)
• 🛠️ Bug fix (non-breaking change which fixes an issue)
• ❌ Breaking change (fix or feature that would cause existing functionality to change)
• 🧹 Code refactor
• ✅ Build configuration change
• 📝 Documentation
• 🗑️ Chore

[kelvinwieth]

Some files were formatted when I ran npm install. Is that acceptable, or did I make something wrong?

[alestiago]

As per the documentation, minCoverage is optional and defaults to 100.

Am I missing something trivial, or would this make the minCoverage argument required?

isNaN(Number(undefined)) // evaluates to True

I'm surprised this isn't being captured by the tests.

[kelvinwieth]

Yeah, you're right. I'll add a new test and fix this.

[kelvinwieth]

Just to mention, that is happening because, when the argument min_coverage is not provided, core.getInput('min_coverage'), returns an empty string ''.

const minCoverage = core.getInput('min_coverage'); // returns ''
let asNumber = Number(minCoverage); // yeah, it returns 0
isNaN(asNumber); // returns false

I love js 😍

[kelvinwieth]

I think this test suit is not right.

First, take a look at the test which 'fails when the coverage is not 100 and min_coverage is not provided'. If we put a breakpoint on index.js 44, we will see that coverage is 100, which is not expected, since the coverage file is the lcov.95.info, and the test description doesn't mention ignoring/excluding files.

This is happening because the previous test, 'completes when the coverage is higher than the threshold after excluding files', is setting the variable INPUT_EXCLUDE, and there are no functions to clean-up the environment variables after each test.

One possible solution is to add the following code before the first test:

afterEach(() => {
  delete process.env['INPUT_PATH'];
  delete process.env['INPUT_EXCLUDE'];
  delete process.env['INPUT_MIN_COVERAGE'];
});

Besides, there's another weird thing on this test file. Let's also take the test 'fails when the coverage is not 100 and min_coverage is not provided' as example:

test('fails when the coverage is not 100 and min_coverage is not provided', () => {
  const lcovPath = './fixtures/lcov.95.info';
  process.env['INPUT_PATH'] = lcovPath;
  const ip = path.join(__dirname, 'index.js');
  try {
    cp.execSync(`node ${ip}`, { env: process.env }).toString();
    fail('this code should fail');
  } catch (err) {
    expect(err).toBeDefined();
  }
});

This is code isn't testing anything.

If we put a breakpoint on catch, we can see that the err error is the one thrown by the call fail('this code should fail');. Instead of asserting it toBeDefined(), we should look for the output with getErrorOutput(err), and then assert the message.

My question is: should I fix these issues with this PR, or should I open another one?

[alestiago]

Hi @kelvinwieth thanks for the details and the work here! I'm happy for you to open two new additional Pull Requests one to include the afterEach and another to perform a check on the error output.

[kelvinwieth]

I will probably be offline in the next days, due to a long weekend, but at least on monday I can open these PRs. Thank you, @alestiago!

[kelvinwieth]

I forgot that my fork branch for this PR was the main, and did reset the changes to work on the afterEach and the error output. After working on these other issues, I will open this fix for the min_coverage on another PR.

Sorry, I'm not experienced with forks to PRs 😳

[alestiago]

No worries, we're all always learning! Thank you for taking the lead here, looking forward to those Pull Requests 🙌