Skip to content

feat , check for min_coverage run time type and show proper error based on the result #258

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

Conversation

maxzod
Copy link

@maxzod maxzod commented Jun 27, 2023

Description

check for min_coverage run time type and show proper error based on the result
#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

@alestiago
Copy link
Contributor

Hi @maxzod ! Thanks for the contribution, can you include some tests to? 💙

@maxzod
Copy link
Author

maxzod commented Jun 28, 2023

@alestiago sure will add them ASAP

@alestiago
Copy link
Contributor

alestiago commented Jul 10, 2023

Hello @maxzod 👋 💙 , thanks for contributing (once again 😄)!

Are you still planning to work on this 👀? There's no rush, I just wanted to check on the status of this Pull Request.

@maxzod
Copy link
Author

maxzod commented Jul 11, 2023

sure , I am still planing to do it

Comment on lines +111 to +118
if (typeof minCoverage == 'number') return true;

if (minCoverage.toString().includes('%')) {
core.setFailed('❌ Failed to use min_coverage remove the `%` symbol');
return false;
}
core.setFailed('❌ Failed to use min_coverage value make sure you added a number');
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Should we not just parse the value as a int and see if we can? Instead of having "checks" for different cases

Copy link
Author

Choose a reason for hiding this comment

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

@wolfenrain I don't get your point!

@maxzod
Copy link
Author

maxzod commented Jul 29, 2023

i think i need some help to figure out how to write a test for this code!
I am familiar with unit tests but not familiar with js environment

@BeatriceMitchell BeatriceMitchell linked an issue Aug 14, 2023 that may be closed by this pull request
@alestiago
Copy link
Contributor

Hi @maxzod , sorry for the late reply. Do you still need help with this?

@alestiago alestiago added the waiting for response Waiting for follow up label Aug 18, 2023
@maxzod
Copy link
Author

maxzod commented Aug 19, 2023

@alestiago , yes
i need example or something to help me write tests for it

@alestiago
Copy link
Contributor

alestiago commented Aug 21, 2023

Hi @maxzod, from our side, we should have a CONTRIBUTING file to help new contributors, I've made a separate issue to tackle this #274 (feel free to drop a 👍 if you think it would be helpful ).

Regardless, I'll try my best on providing some guidance.

  • All tests are in a single file index.test.js
  • The project uses Jest a JavaScript testing framework, you can run all current tests if you have node installed via npm test (make sure to install dependencies first via npm i)
  • You can specify the min_coverage input using:
// Specify input:
process.env['INPUT_MIN_COVERAGE'] = 85;
// Run the tool:
cp.execSync(`node ${ip}`, { env: process.env }).toString();
  • You can check if there are errors with a try catch and Jest, doing something as:
 try {
    cp.execSync(`node ${ip}`, { env: process.env }).toString();
    fail('this code should fail');
  } catch (err) {
    expect(err).toBeDefined();
  }
  • This test should provide a nice boilerplate to do what we want to check here (that an error is thrown when the min coverage is not a number)
  • Fixtures are real pre-computed lcov files used to test the tool, in this case you shouldn't require any fixture since the check should run before the lcov file is analysed

I hope these bullet points help you out! If you have any further questions feel free to drop them here 💙

@alestiago alestiago assigned alestiago and unassigned wolfenrain Aug 21, 2023
@alestiago alestiago marked this pull request as draft September 4, 2023 08:06
@alestiago
Copy link
Contributor

@maxzod did the test guidance help? Any updates here? Do you still have bandwidth to work on this?

@alestiago
Copy link
Contributor

Hi @maxzod I'll be closing this Pull Request, since we usually give a two week period of inactivity before closing. Feel free to re-open if you would like to continue working on it! Thank you 💙 🙌

@alestiago alestiago closed this Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for response Waiting for follow up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: catch non-numeric min_coverage input
3 participants