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

Update integration tests to also check Get-TargetResource #4470

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

Borgquite
Copy link
Contributor

Pull Request (PR) description

Have seen a few bugs myself where Get-TargetResource is failing (#2775) and sometimes this causes unexpected behaviour in the rest of the resource (#4437)

Hopefully this adds a simple sanity check using Get-DSCConfiguration to the integration tests to catch these before release

This Pull Request (PR) fixes the following issues

@ricmestre
Copy link
Contributor

But why? Test-DSCConfiguration calls the associated Test-TargetResource function of the resources in the config which in turn already call Get-TargetResource for each one of them.

@Borgquite
Copy link
Contributor Author

@ricmestre If I'm honest, it's a desperate attempt to try to improve the integration tests on this resource, a struggle that I'm aware you also share! (#3458). At the moment I'm dreading the times when I have to update Microsoft365DSC - let alone start to try to a new resource - because there's absolutely no guarantee that it's not going to break in a new, and unexpected way.

Sometimes, that new and unexpected way is, for example, going to wipe out entire sections of my Microsoft 365 tenant without warning me (#3194).

I'd describe myself as 'intermediate' at DSC, perhaps slightly less than 'intermediate' when it comes to module writing. The reasonf for this is that I tried to get my head around #4437 yesterday, and it turned out that the cause was a try {} catch loop that had been incorrectly duplicated - an error that wasn't noticed by the author, or the approver, or the existing integration or unit tests. I figure that if that sort of error can fall through the gaps in this project, it's worth testing Get-TargetResource itself, as I think an incorrect try {} catch around Get-TargetResource in Test-TargetResource would possibly fall through the gaps.

But mainly I'm just grasping around for something, anything, that would stop my headaches with this vital but sorely inadequately tested module.

@ricmestre
Copy link
Contributor

I understand since I found myself in the same position too many times and I'm not even using it in production yet, but in this case this particular PR won't solve much, in fact it's a duplication of work that isn't necessary.

Currently when something is merged into the repo it will run these workflows you changed, you can have a look into "Actions" (https://github.com/microsoft/Microsoft365DSC/actions) and look for "Global - Integration - {AAD,EXO,INTUNE}" and their output, there are resources that cause errors and impact the whole workflow, you can look at those.

Also, the tests these workflows call are present in folder "Modules\Microsoft365DSC\Examples\Resources" so you can look at what's being tested.

@Borgquite
Copy link
Contributor Author

@ricmestre As mention, I wouldn't use or update any resource here without extensive testing of the entire CRUD process for each variable, and I've learned the hard way that I need to do a line-by-line code review of each change before deploying any update. Even then I am missing bugs (like #4437 where I'm fairly sure before the changes, Get-TargetResource was just returning whatever was passed to it without even checking Microsoft Graph, therefore breaking Test-TargetResource).

If I run the following script, then the second catch {} doesn't fire. Doesn't that mean there is value in this patch?

# Test-TargetResource
try {
    write-host "hello"
    try {
        # Buggy Get-TargetResource here
        throw "hi"
        
    }
    catch
    {
        write-host "caught"
    }
}
catch
{
    write-host "not caught"
}

On the integration tests, it doesn't look like they fire on every merge, only some? For example, this one https://github.com/microsoft/Microsoft365DSC/actions/runs/8344518121 doesn't appear to have any integration test runs associated with it? Am I missing something?

@ricmestre
Copy link
Contributor

@NikCharlebois @andikrueger Could you please check this?

@NikCharlebois
Copy link
Collaborator

I will merge the changes because there is indeed value in them. These "Global -" tests are set to be replaced by the new suite of integration tests as time permits. Currently the tests have been put in place for AAD, and partially for EXO.

@NikCharlebois NikCharlebois merged commit 9e39534 into microsoft:Dev Mar 21, 2024
2 checks passed
@Borgquite
Copy link
Contributor Author

Borgquite commented Mar 21, 2024

@NikCharlebois Thanks so much - appreciated!

Please could you let us know if there's a strong reason why the existing integration tests don't run on pull requests? (Looks like only pushes?)

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