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

SqlServerAudit #1433

Closed
wants to merge 17 commits into from
Closed

SqlServerAudit #1433

wants to merge 17 commits into from

Conversation

Fiander
Copy link
Contributor

@Fiander Fiander commented Sep 26, 2019

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
    Entry should say what was changed, and how that affects users (if applicable).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is Reviewable

@codecov-io
Copy link

codecov-io commented Sep 27, 2019

Codecov Report

Merging #1433 into dev will decrease coverage by 7%.
The diff coverage is 1%.

Impacted file tree graph

@@          Coverage Diff          @@
##            dev   #1433    +/-   ##
=====================================
- Coverage    98%     90%    -8%     
=====================================
  Files        38      40     +2     
  Lines      5603    6085   +482     
=====================================
+ Hits       5497    5503     +6     
- Misses      106     582   +476

@johlju
Copy link
Member

johlju commented Sep 27, 2019

@Fiander Awesome work! 😃 Let me know when this is ready for review.

@Fiander
Copy link
Contributor Author

Fiander commented Sep 27, 2019

At the moment i am combining this and a home renovation. so slow progress.
stil trying to get the code past appveyor, and then not making the codecov drop 8%.

these are only 2 out of a 3 project.
Audit ( the storage ) & ServerAuditSpecification.

with a few months DatabaseAuditSpecification should be ready.
but first i want to finish my home.

@johlju
Copy link
Member

johlju commented Sep 27, 2019

No worries, there a not hurry. 🙂 Sounds like a good prioritization. 😄 Tag me if I can help with the anything. Well, in the resource, not the home renovation. 😉

@Fiander
Copy link
Contributor Author

Fiander commented Sep 27, 2019

made some last changed so it now should pass the meta checks.
the unit tests are a work in progress, but that can take two weeks.

p.s. could really use some help on the house ;-)

jpomfret and others added 2 commits October 14, 2019 18:16
- Changes to SqlSetup
  - Fix typo in SqlSetup strings (issue #1419).
…nd fix hashtables (#1447)

- Changes to SqlServerDsc
  - Add .gitattributes file to checkout file correctly with CRLF.
  - Updated .vscode/analyzersettings.psd1 file to correct use PSSA rules
    and custom rules in VS Code.
  - Fix hashtables to align with style guideline (issue #1437).
- Changes to SqlServerMaxDop
  - Fix line endings in code which did not use the correct format.
@Fiander
Copy link
Contributor Author

Fiander commented Dec 6, 2019

Hi, I finally gotten far enough with the house so I can continu with this.
I completed the integration test for the ServerAudit, and started with the integration tests for the ServerAuditSpecification. they wil be ready soon.

but I am absolutely not experienced enough for the unit testing. I truly have no clue on where to start and what to do. Are they really needed? I read trough some of the others, and they seem to do the same as the integration tests but not to an actual server but to some fake smo objects who are pretending to be an actual SQL Server? is this correct?

@johlju johlju changed the base branch from dev to master January 1, 2020 20:58
@johlju
Copy link
Member

johlju commented Jan 3, 2020

Sorry that I have not answered for a long time, been busy getting the new CI pipeline working for the DSC Community.

Unit tests is needed. Without them we don't merge changes. Unit tests can verify that all code paths work, which can be harder to do with integration tests some tests might not even be able, or harder, to be tested, like throwing an error or changing the state of the build worker in such a manner that it cannot recover. Unit tests also shows us the code coverage of the tests where integration tests does not since the LCM is running the tests. Unit tests mocks the actually action and instead returns a state, the unit test verifies that the state returned is the one that was expected. Normally you mock cmdlets, but since the are so few cmdlets for SQL Server we have to resort to using the SMO in the code. That means that sometimes we have to mock the SMO classes, or make wrapper functions for the SMO calls that in turn we can mock easily. Those wrappers have minimal code (one-liners) and does not need to be unit tested.

The new CI pipeline has merged in this repository. This changed the folder structure, and also removed the dev branch. Please rebase against the master branch, and make sure to get your changes into the the file in the new location (under source folder). 😃

Read more here about the new coding workflow:
https://dsccommunity.org/guidelines/contributing/#understand-the-coding-workflow
https://dsccommunity.org/guidelines/testing-guidelines/#running-tests
You can also use Invoke-Pester once you run build.ps1 -Task build (not documented yet)
https://dsccommunity.org/guidelines/contributing/#attach-your-fork-to-a-free-azure-devops-organization

Let me know if you need further help.

@Fiander Fiander closed this Jan 13, 2020
@johlju johlju mentioned this pull request Apr 16, 2020
9 tasks
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.

4 participants