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

xWebsite: Compare LogTruncateSize parameter correctly #381

Closed
wants to merge 3 commits into from

Conversation

nickgw
Copy link
Contributor

@nickgw nickgw commented Jul 12, 2018

xWebsite attempts to compare the passed $LogTruncateSize to (essentially) (Get-Website -Name $name).logFile.LogTruncateSize except, the parameter is truncatesize. This causes the comparison to always fail.

Fixes #380


This change is Reviewable

@msftclas
Copy link

msftclas commented Jul 12, 2018

CLA assistant check
All CLA requirements met.

@johlju
Copy link
Member

johlju commented Jul 12, 2018

Could you please look why the tests are failing?

@johlju johlju added the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jul 12, 2018
@nickgw
Copy link
Contributor Author

nickgw commented Jul 12, 2018

Looks like there was something else going on at the same time:
The process cannot access the file 'C:\windows\system32\inetsrv\config\applicationHost.config' because it is being used by another process.

I'm not sure how to trigger the appveyor build again.

@johlju
Copy link
Member

johlju commented Jul 12, 2018

Ah some timing issue, think I’ve seen that before. Closing the PR and directly reopening the existing PR again will kick off the tests again.

@nickgw nickgw closed this Jul 12, 2018
@nickgw nickgw reopened this Jul 12, 2018
@codecov-io
Copy link

codecov-io commented Jul 12, 2018

Codecov Report

Merging #381 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #381   +/-   ##
=======================================
  Coverage   90.44%   90.44%           
=======================================
  Files          17       17           
  Lines        2450     2450           
=======================================
  Hits         2216     2216           
  Misses        234      234
Impacted Files Coverage Δ
DSCResources/MSFT_xWebsite/MSFT_xWebsite.psm1 95.63% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a07d02c...e739797. Read the comment docs.

@johlju johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jul 12, 2018
@johlju johlju changed the title xWebsite: Compare LogTruncateSize parameter correctly (Fixes #380) xWebsite: Compare LogTruncateSize parameter correctly Jul 17, 2018
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nickgw)

a discussion (no related file):
Could you please look at the unit tests for this resource and figure out why (unless you already know) the unit tests passed with the wrong parameter name LogTruncateSize? We need to add a regression test that tests this change.


@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jul 17, 2018
@nickgw
Copy link
Contributor Author

nickgw commented Jul 18, 2018

I believe it is because the unit tests always check for failure scenarios but never successful scenarios. If I'm reading the tests right:

L269 test that they match, calling the right parameter names (LogTruncateSize for passed parameter and truncatesize for website parameter),

L681 test for failure when the truncatesize is different, and

L777 tests for failure when LogTruncateSize is larger than truncatesize.

L1524 isn't testing for comparison between the two (though I'm not the best with Pester).

If there was a test that the given LogTruncateSize matches truncatesize looking for a True, this would have been caught.

@johlju
Copy link
Member

johlju commented Jul 19, 2018

When looking at the lines you provided, it seems that the unit test was mocking this correctly, so the unit tests always tests the correct property.

https://github.com/PowerShell/xWebAdministration/blob/a07d02c295f49efd5a608f0915a7dfdf19d6da2b/Tests/Unit/MSFT_xWebsite.Tests.ps1#L102-L122

https://github.com/PowerShell/xWebAdministration/blob/a07d02c295f49efd5a608f0915a7dfdf19d6da2b/Tests/Unit/MSFT_xWebsite.Tests.ps1#L665

We should test this property in the integration test, where it would have been caught, since there Get-WebSite is not mocked.

@johlju
Copy link
Member

johlju commented Jul 19, 2018

No I take that back. This should have been caught in the unit tests. Something is off with the unit test. It mocked the return value as, for example, truncateSize = '1048576' , and that could never have been returned, since when it previously returned LogTruncateSize it would have returned a $null value. 🤔

Using the parameters you used to hit this bug, could you with those create a new unit test for each function, that catch this bug (and with you fix the tests passes)?

@stale
Copy link

stale bot commented Aug 2, 2018

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@stale stale bot added the abandoned The pull request has been abandoned. label Aug 2, 2018
@johlju johlju changed the base branch from dev to master December 30, 2019 12:18
@johlju johlju changed the base branch from master to main January 7, 2021 19:15
@nickgw nickgw closed this Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned The pull request has been abandoned. waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xWebsite: LogTruncateSize is not a property of (Get-Website).LogFile
4 participants