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

WIP/Tests | Widen test code coverage #3086

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

edwardneal
Copy link
Contributor

WIP - running this through CI and Codecov to make sure my local coverage figures align.

This PR ensures that the public API surface for SqlDataRecord and SqlMetaData are covered by tests. It also expands coverage of some of the basic data validation on SqlParameter and SqlCommand.

@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 21, 2024

@edwardneal Did you look at the code coverage report for "inspiration" ?

@edwardneal
Copy link
Contributor Author

I did; Codecov aligns with it:

The code coverage for SqlDataRecord is currently ~75%, where I expect this PR will bring it to close to 100% in the code coverage report. The code paths which aren't covered are ones which can't ever be called and are due to be removed; once that happens I expect these two files will hit 100%.

SqlParameter is a much larger class, and it's got a custom TypeConverter which doesn't appear to have test coverage. I've added a few tests for basic validation, but the class will need more work here.

@edwardneal
Copy link
Contributor Author

@mdaigle would you mind triggering the Azure Pipelines bot please? It looks like CI has stopped triggering on my PRs.

@mdaigle
Copy link
Contributor

mdaigle commented Jan 2, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@edwardneal
Copy link
Contributor Author

Thanks. It looks like there's a slightly deeper problem with CI - the Password and the Azure_Password variables (and maybe other secret variables?) aren't being substituted.

@mdaigle
Copy link
Contributor

mdaigle commented Jan 2, 2025

Thanks. It looks like there's a slightly deeper problem with CI - the Password and the Azure_Password variables (and maybe other secret variables?) aren't being substituted.

Yeah, I need to dig into this further. We had a security action item to limit forked build access to pipeline secrets and I think the trigger was pulled on that before we made accommodating changes. There's a general trend toward locked down permissions for forked builds. We need to start updating tests to use dynamically generated (rather than static) resources so that we don't rely on pipeline secrets.

@David-Engel started doing some work in this direction with #2750. I bumped the pipeline there to see if mac tests are passing with those changes.

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.

3 participants