Restore Code Coverage Verification in GitHub Actions Workflow

[RenanCarlosPereira]

Description:

The code coverage verification step was previously part of our GitHub Actions build process but has been removed. This step is crucial for maintaining high code quality and ensuring adequate test coverage. We need to investigate why it was removed and reintegrate it into our CI/CD pipeline.

Steps to Reproduce:

1. Review the history of the GitHub Actions workflow configuration to identify when and why the code coverage step was removed.
2. Assess the impact of its removal on the code quality and test coverage.

Expected Behavior:

• The GitHub Actions workflow should include a step to verify code coverage using the dotnet reportgenerator tool.
• The code coverage report should be generated and accessible in the build artifacts.

Actual Behavior:

• The code coverage verification step is missing from the current workflow.

[asulwer]

i could not see a valid purpose for codecoverage. using features just because they exist doesn't make a lot of sense. was it useful in someway? i couldn't see it.

How? "crucial for maintaining high code quality and ensuring adequate test coverage"

[RenanCarlosPereira]

i could not see a valid purpose for codecoverage. using features just because they exist doesn't make a lot of sense. was it useful in someway? i couldn't see it.

How? "crucial for maintaining high code quality and ensuring adequate test coverage"

Keeping code coverage in open source projects is super important for several reasons. First off, it helps catch bugs early. By writing tests for your code, you can find and fix issues before they become a problem for users. This makes the project more reliable and trustworthy, which is crucial for attracting and keeping users and contributors.

When contributors, especially new ones, see that the project has good test coverage, they feel more confident in making changes or adding new features. They know that if something goes wrong, the tests will catch it. This makes it easier for people to contribute without the fear of breaking existing functionality.

Good test coverage also helps keep the codebase clean and maintainable. As the project evolves, having a solid suite of tests ensures that new changes don’t introduce bugs. It also makes refactoring code safer and more manageable because you can rely on the tests to verify that everything still works as expected.

Take this project as example, you only are able to work comfortable because of the level of the coverage that this codebase represent nowadays.

Moreover, tests serve as a form of documentation. They show how different parts of the code are supposed to work, which is incredibly helpful for anyone trying to understand the codebase, especially newcomers.

By integrating tests with continuous integration (CI) tools, you can automate the quality checks. This means that every time someone makes a change, the tests run automatically, ensuring that the code quality remains high.

In short, maintaining good code coverage keeps the project healthy, makes it easier and safer to contribute, and ensures that the code remains reliable and understandable. It’s a win-win for both the maintainers and the contributors.

also could you add me as a maintainer? merge to main branches needs to be well explained in the PR's. I've noticed some commits without a good reason 😥

the ones from sign-keys... the project from Microsoft already had it in place, not sure why you changed it.

[asulwer]

i am not saying no to anything but i really need to understand the why.

if i understand correctly, please excuse my ignorance, but testing has not been removed. github/workflows/dotnetcore-build.yml runs on every push/pull and the last line is Test. that line runs all of the UnitTests and complains on failures

are you suggesting adding after the Test line the additional items i removed? Generate Report, Check Coverage, Coveralls GitHub Action?

i removed the Check Coverage line because it failed every time

[asulwer]

??? https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-code-coverage?tabs=windows

[RenanCarlosPereira]

Yes, so if anyone implements new feature and the coverage of the code drops the pull request wont complete, maintaining the good quality gate and coverage of the lines

[asulwer]

yes i see the need for this. i am readding it.

[RenanCarlosPereira]

If you notice this line

It says we need to maintain 96% of coverage in the original project

 https://github.com/microsoft/RulesEngine/blob/dc5298989583954cd6aac598267747b4ce635f45/.github/workflows/dotnetcore-build.yml#L34

[asulwer]

cover coverage complains about a lot of the original code. i am going to start focusing on some of these issues.

[RenanCarlosPereira]

I can work on it once I get home, Could you please hold on the changes in a branch so we can review the pRs together?

[RenanCarlosPereira]

Also before fixing the issues, we need to understand why is it complaining as the previous version in Microsoft repository it was passing normally 👍🏾

[asulwer]

the ones from sign-keys... the project from Microsoft already had it in place, not sure why you changed it.

the public key was published to the repository but not the private key, so technically it was not signing release builds. this has been fixed and the private key remains unpublished

[asulwer]

the workflow, as expected, is failing due to codecoverage. the original existing code is filled with issues, according to codecoverage

[RenanCarlosPereira]

please do not complete the Pull Request if the workflow is not passing, I will have to reopen this one.

we need to guarantee that 96% of the code is covered, for this build it failed.

sorry

https://github.com/asulwer/RulesEngine/pull/12

[asulwer]

the original branch failed with this too which was my original reason for removing.

[RenanCarlosPereira]

Im fixing it, give me a second, will open a PR

they were all passing in the original project. that's not passing anymore because you added new methods to the interface, and they were not tested

https://github.com/microsoft/RulesEngine/actions/workflows/dotnetcore-build.yml

[asulwer]

i have it working up to the point of Check Coverage

[asulwer]

i need unit tests for the new code? which is why its failing?

[RenanCarlosPereira]

yes, your new code is not tested, so it dropped the quality, and then it's not passing.

You should roll back the changes in this file, and keep it as it was. In the future, we can add more methods to that interface.

the ones you marked as obsolete should not be obsolete, as they are working fine.

https://github.com/microsoft/RulesEngine/blob/main/src/RulesEngine/RulesEngine.cs https://github.com/microsoft/RulesEngine/blob/main/src/RulesEngine/Interfaces/IRulesEngine.cs

[asulwer]

i agree that the obsolete methods are working fine but the reason for change is they are confusingly named. they also do not support the solved issues that have been brought up. they were only marked obsolete to prevent breaking changes. their use is confusing and not appropriately named for what they accomplish. RulesEngine file is confusing just to navigate and understand what is going on partly due to naming conventions that have been used.

[RenanCarlosPereira]

To be honest, I suggest we revert the changes made in this fork, focusing solely on bug fixes. The community must trust this fork, and introducing numerous changes and new features might undermine that trust. In an open-source project, maintaining stability and reliability is paramount.

Therefore, our priority should be to address the existing bugs without introducing new features at this time. This approach ensures we do not disrupt the current user base and build confidence in the project's reliability.

[RenanCarlosPereira]

I understand the method names can be confusing, but many users are familiar with them as they are. Changing them now might just add to the confusion and disrupt workflows.

Moreover, the new names haven't been properly tested yet. Rushing these changes could lead to issues and erode trust in the new implementation. It's nice to maintain stability and ensure thorough testing before making any significant changes. Let's focus on clarity and reliability to keep user confidence high.

I would suggest to focus only in the bug fixes, even creating a new fork, and open PR's from scratch solving each problem at time.

let me know what you think

[asulwer]

i agree with what you are saying, really i do. i rushed some of the changes that i made and should have created PR's for those changes.

[RenanCarlosPereira]

I know some big companies use this repo, and seeing these changes reviewed by only one contributor is a bit messy.

If you want, I can fork the repo myself and add you as a contributor. This way, we can implement each change with a clear rationale behind it. We can also invite more people to participate, but all new features or disruptive changes should be discussed in the forum within the fork before moving forward. This approach ensures transparency and builds trust in the process.

@abbasc52, requested us and I would like to keep it. https://github.com/microsoft/RulesEngine/issues/604#issuecomment-2184408178

Also, @timophe-91 has some concerns about the direction as he mentioned it is a working environment, I do have some concerns too as I have it in my production environment that could affect a large number of people if not tested properly.

let me know your thoughts on this 🙌

[asulwer]

i want to thank you for the time you take to completely answer my questions. i will revert the obsolete code back.

[asulwer]

mostly reverted back. i removed all of the methods i added plus the obsolete attribute.

[RenanCarlosPereira]

Nice man, tomorrow morning I will do a comparison with the original code so we can do a descriptive change, thanks for that 🙂

[asulwer]

main branch has some differences but it passed, barely.

[RenanCarlosPereira]

Great, I'll compare the changes right now and create a PR. Once that's done, I'll merge it soon. Moving forward, we'll open pull requests with detailed explanations and rationales for each implementation. This will ensure clarity and transparency in our development process.

[asulwer]

i should have done that from the beginning, moving forward i will. thank you for the assistance and taking the time to provide detailed thoughts

[asulwer]

once you have gone through main (and approved) i will push a nuget package

[RenanCarlosPereira]

The PR is open reverting the changes maintained the bug fixes, also keeping the codeql code coverage and unit tests levels @asulwer Check it out

[asulwer]

i had an essay prepared for you with the many issues i believe existed in that PR. but..i can create a pull request with my changes to it.

[abbasc52]

On code coverage, it served as a good check for new changes being tested. On unit tests(they are more like functional tests) they helped ensure bugs didnt pop up again, So any bug reported would become a test case so that future changes dont break it again.

You can reduce the percentage in coverage check till you guys stabilize the repo.

On signing, i would recommend generating new signing( it was used for a feature called 'strongname'. It is not mandatory, it just limits few people who are using strongname restriction in their project from using non signed nugets

[RenanCarlosPereira]

Nice, we will create PRs from now on, would be great to complete them always with a second pair of eyes.

@abbasc52 would be nice if you could review them, I know you cant maintain it or anything but we would appreciate if you could review them sometimes and add some comments if needed just by the time we make it stable 😀

[RenanCarlosPereira]

@asulwer lets open PRs with specific changes

If you could open it and don't complete until theres another pair of eyes looking to it.

Would be great to add the policy so minimal of 2 reviews in order to complete the PR into main

[asulwer]

in the future we can adjust the policy to include more reviewers. as we have just you and i lets keep it to one. right now the merged PR has broken the solution. before any packages are fixed the issues need to be solved. we need a stable branch with discussion about what's to be removed and what should be kept. right now i am not feeling good about this fork

[RenanCarlosPereira]

We can release a version, no worries about it, because we have it almost exactly the way in the original package 😀

I make sure we didn't change any behavior, I just added the cancellation token from in the context, so the users and use it

[RenanCarlosPereira]

How did it broken the solution the build passed together with three test and coverage and codeql? 🧐

[asulwer]

look at the solution using (windows File Explorer) old code and new code has been mixed

[RenanCarlosPereira]

Noo, look it on github, you need to clone it back again those files are only in your machine

[asulwer]

[asulwer]

i have a PR open with my corrections linking to open issues for each correction

[asulwer]

this issue is now off topic. i have created issues for 'some' of the issues found with the merge