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.

Sign up for GitHub

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

Jump to bottom

(#2893) Switch from Should to FluentAssertions #2908

Merged
merged 4 commits into from
Jun 6, 2023
Merged

(#2893) Switch from Should to FluentAssertions #2908

merged 4 commits into from
Jun 6, 2023

Conversation

TheCakeIsNaOH
Copy link
Member

@TheCakeIsNaOH TheCakeIsNaOH commented Nov 18, 2022 •
edited by gep13
Loading

Description Of Changes

This switches chocolatey.tests and chocolatey.tests.integration to use FluentAssertions instead of Should. This is because Should is no longer actively maintained, so it is time to replace it.

As per the FluentAssertions api, various calls needed to be adjusted, mostly in the pattern Shouldxxxx() to Should().xxxx()

Motivation and Context

See #2893

Testing

Ran unit and integration tests with build.ps1 and in Visual Studio

Change Types Made

  • Bug fix (non-breaking change)
  • Feature / Enhancement (non-breaking change)
  • Breaking change (fix or feature that could cause existing functionality to change)
  • PowerShell code changes.

Related Issue

Change Checklist

  • Requires a change to the documentation
  • Documentation has been updated
  • Tests to cover my changes, have been added
  • All new and existing tests passed.
  • PowerShell v2 compatibility checked.

@TheCakeIsNaOH
Copy link
Member Author

It looks like FluentAssertions 6.x is not compatible with Visual Studio 2017.
https://stackoverflow.com/questions/68911113/ambiguous-call-when-using-should-notbenull-on-as-item

Since I have Visual Studio 2017 installed locally, the build.ps1 fails, as it tries to use MSBuild from vs 2017. However, I can build with Visual Studio 2022. That is why this is currently in draft.

@gep13
Copy link
Member

gep13 commented Nov 18, 2022

@TheCakeIsNaOH said...
It looks like FluentAssertions 6.x is not compatible with Visual Studio 2017.
https://stackoverflow.com/questions/68911113/ambiguous-call-when-using-should-notbenull-on-as-item

What happens if you change the langVersion in the csproj file to 7.3?

@TheCakeIsNaOH
Copy link
Member Author

@gep13 that fixed it.
I've pushed a commit with the change.

@TheCakeIsNaOH TheCakeIsNaOH force-pushed the gh2893-fluent-assertions branch 4 times, most recently from 5130303 to af4ba7b Compare November 23, 2022 01:07
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the gh2893-fluent-assertions branch from af4ba7b to d0767a4 Compare December 13, 2022 00:56
@TheCakeIsNaOH TheCakeIsNaOH marked this pull request as ready for review December 13, 2022 00:57
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the gh2893-fluent-assertions branch from d0767a4 to 00054e7 Compare December 13, 2022 01:14
AdmiringWorm
AdmiringWorm requested changes Dec 14, 2022
View reviewed changes
Copy link
Member

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

I have done a partial review to have something to get started on, and similar changes would be needed for the rest of the files.

src/chocolatey.tests.integration/infrastructure.app/services/FilesServiceSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/infrastructure/filesystem/DotNetFileSystemSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/infrastructure/filesystem/DotNetFileSystemSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/infrastructure/filesystem/DotNetFileSystemSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/infrastructure/filesystem/DotNetFileSystemSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/scenarios/InstallScenarios.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/scenarios/InstallScenarios.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/scenarios/InstallScenarios.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/scenarios/InstallScenarios.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/scenarios/InstallScenarios.cs Outdated Show resolved Hide resolved
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the gh2893-fluent-assertions branch 3 times, most recently from 60db09d to 85be439 Compare December 16, 2022 23:29
@choco-bot
Copy link

Task linked: PROJ-427 Switch Should to FluentAssertions

@TheCakeIsNaOH TheCakeIsNaOH force-pushed the gh2893-fluent-assertions branch 3 times, most recently from 1e0ebff to a37fb56 Compare December 22, 2022 00:39
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the gh2893-fluent-assertions branch from a37fb56 to e961b2c Compare January 6, 2023 16:36
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the gh2893-fluent-assertions branch 11 times, most recently from 43fa9d8 to dc382f7 Compare June 4, 2023 03:24
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the gh2893-fluent-assertions branch 2 times, most recently from 1c1b580 to 3cf3150 Compare June 4, 2023 03:39
@TheCakeIsNaOH
Copy link
Member Author

This is ready for a re-review, I think I completed the suggested changes across the various test files.

AdmiringWorm
AdmiringWorm requested changes Jun 5, 2023
View reviewed changes
Copy link
Member

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

Some items I think should be fixed so that we have a reference when adding new tests.

Do note that there are quite a few other places that will benefit for similar changes that I haven't commented on, however only the ones I have commented on are required to be fixed at this moment.

src/chocolatey.tests.integration/infrastructure/filesystem/DotNetFileSystemSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/infrastructure/filesystem/DotNetFileSystemSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/infrastructure/filesystem/DotNetFileSystemSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/scenarios/InfoScenarios.cs Outdated Show resolved Hide resolved
src/chocolatey.tests.integration/scenarios/InfoScenarios.cs Outdated Show resolved Hide resolved
src/chocolatey.tests/App.config Outdated Show resolved Hide resolved
src/chocolatey.tests/infrastructure.app/services/RulesServiceSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests/infrastructure.app/services/RulesServiceSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey.tests/infrastructure/filesystem/DotNetFileSystemSpecs.cs Outdated Show resolved Hide resolved
src/chocolatey/app.config Outdated Show resolved Hide resolved
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the gh2893-fluent-assertions branch from 3cf3150 to 525fc47 Compare June 6, 2023 01:23
@TheCakeIsNaOH TheCakeIsNaOH force-pushed the gh2893-fluent-assertions branch 2 times, most recently from 0d18d5a to 1a35a27 Compare June 6, 2023 02:16
@AdmiringWorm AdmiringWorm force-pushed the gh2893-fluent-assertions branch from 1a35a27 to 7b3f795 Compare June 6, 2023 09:56
AdmiringWorm
AdmiringWorm previously approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@AdmiringWorm AdmiringWorm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, but had to downgrade the System.Runtime.CompilerServices.Unsafe dependency as it causes problems with the build system, and we want to keep it closer to the version in other projects.

EDIT: Assuming this successfully builds now, we'll get this merged.

@TheCakeIsNaOH @AdmiringWorm
(chocolatey#2893) Switch from Should to FluentAssertions
3bdeb93 
This switches chocolatey.tests and chocolatey.tests.integration
to use FluentAssertions instead of Should. This is because Should
is no longer actively maintained, so it is time to replace it.

The Fluent Assertions Analyzers are also added in this commit.

Only the package and dependencies are switched. The code
changes be in the next commit.
@TheCakeIsNaOH @AdmiringWorm
(chocolatey#2893) Update references and syntax to FluentAssertions
9d5b05c 
As per the FluentAssertions api, various calls needed to be adjusted,
mostly in the pattern Shouldxxxx() to Should().xxxx()
@TheCakeIsNaOH @AdmiringWorm
(chocolatey#2893) Switch Unit and Integration tests to langVersion 7.3
51efab5 
This is to allow Visual Studio 2017 to build the project.
Otherwise, code in the FluentAssertions library will cause errors when build with VS2017.
@TheCakeIsNaOH @AdmiringWorm
(chocolatey#2893) Update tests to use to FluentAssertions style
c853166 
This updates tests to use new ways of writing assertions that are available in
FluentAssertions but where not in Should
@AdmiringWorm AdmiringWorm force-pushed the gh2893-fluent-assertions branch from 7b3f795 to c853166 Compare June 6, 2023 11:01
AdmiringWorm
AdmiringWorm approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

LGTM

@AdmiringWorm AdmiringWorm merged commit 5961b94 into chocolatey:develop Jun 6, 2023
@AdmiringWorm
Copy link
Member

@TheCakeIsNaOH your changes have been merged. Thanks for your contribution 👍

@TheCakeIsNaOH TheCakeIsNaOH deleted the gh2893-fluent-assertions branch June 6, 2023 21:48
Meir017 added a commit to fluentassertions/fluentassertions.analyzers that referenced this pull request Aug 15, 2023
@Meir017
Update README.md
652f415 
add @TheCakeIsNaOH chocolatey/choco#2908
@Meir017 Meir017 mentioned this pull request Aug 15, 2023
Merged
Meir017 added a commit to fluentassertions/fluentassertions.analyzers that referenced this pull request Aug 15, 2023
@Meir017
docs: add more example usages (#210)
6ae598f 
* docs: add more example usages

* Update README.md

add @TheCakeIsNaOH chocolatey/choco#2908
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AdmiringWorm AdmiringWorm AdmiringWorm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate from Should to FluentAssertions
4 participants
@TheCakeIsNaOH @gep13 @choco-bot @AdmiringWorm