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

test: Remove moq library #141

Merged
merged 15 commits into from
Aug 11, 2023
Merged

test: Remove moq library #141

merged 15 commits into from
Aug 11, 2023

Conversation

askpt
Copy link
Member

@askpt askpt commented Aug 9, 2023

Remove Moq library

  • This PR removes the Moq library from the test projects. Now it uses NSubstitute.

Related Issues

Fixes #140

Notes

Follow-up Tasks

How to test

@askpt askpt changed the title Remove moq library test:Remove moq library Aug 9, 2023
@askpt askpt changed the title test:Remove moq library test: Remove moq library Aug 9, 2023
@benjiro
Copy link
Member

benjiro commented Aug 9, 2023

@askpt I'm fine with this going a head, thanks for doing it 👍🏻

Are you able to look into the failing steps

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #141 (b0aa44a) into main (15473b6) will decrease coverage by 0.74%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #141      +/-   ##
==========================================
- Coverage   94.64%   93.91%   -0.74%     
==========================================
  Files          20       20              
  Lines         542      542              
  Branches       39       39              
==========================================
- Hits          513      509       -4     
- Misses         16       20       +4     
  Partials       13       13              

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@benjiro
Copy link
Member

benjiro commented Aug 10, 2023

Aah looks like the build is breaking for other reasons, ill take a look this evening after work

@askpt
Copy link
Member Author

askpt commented Aug 10, 2023

@benjiro The library is not fully removed in this PR. There are still a few tests missing that have more advanced scenarios. I don't mind fixing the build issues and creating a new issue for these tests.

Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
Signed-off-by: André Silva <[email protected]>
@benjiro
Copy link
Member

benjiro commented Aug 10, 2023

If you rebase off main it will fix the warnings that are making E2E Test, and Test linux fail.

error EnableGenerateDocumentationFile: Set MSBuild property 'GenerateDocumentationFile' to 'true' in project file to enable IDE0005 (Remove unnecessary usings/imports) on build (https://github.com/dotnet/roslyn/issues/41640) [/home/runner/work/dotnet-sdk/dotnet-sdk/test/OpenFeature.E2ETests/OpenFeature.E2ETests.csproj::TargetFramework=net6.0]

Copy link
Contributor

@bacherfl bacherfl left a comment

Choose a reason for hiding this comment

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

thanks for doing this and great work with the adapted tests so far - I've just had a look and all the assertions we previously had in place are still present.

Copy link
Contributor

@bacherfl bacherfl left a comment

Choose a reason for hiding this comment

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

This looks good to me! For me it would be ok to tackle the remaining test cases in a follow up - @benjiro what do you think?

@askpt
Copy link
Member Author

askpt commented Aug 10, 2023

This looks good to me! For me it would be ok to tackle the remaining test cases in a follow up - @benjiro what do you think?

Pushed more tests now. Only a handful missing. We can merge as it is.

@bacherfl
Copy link
Contributor

This looks good to me! For me it would be ok to tackle the remaining test cases in a follow up - @benjiro what do you think?

Pushed more tests now. Only a handful missing. We can merge as it is.

Thanks! Regarding the failing codecov check: This seems to be caused by a different behavior of NSubstitute vs Moq. The difference is in the Register_Hooks_Should_Be_Available_At_All_Levels in the OpenFeatureHookTest - This was previously covering the exception handling in the Error hook, as Moq mocks throw an exception for methods where no behavior has been defined, while NSubstitute does not seem to do that, therefore we do not reach this catch block in the aforementioned test.
Given that the purpose of this test is just to check if the Register method is available on all levels to register hooks, I think covering the exception handling of the Error hook can be part of a dedicated PR for that

@askpt
Copy link
Member Author

askpt commented Aug 11, 2023

@bacherfl

This looks good to me! For me it would be ok to tackle the remaining test cases in a follow up - @benjiro what do you think?

Pushed more tests now. Only a handful missing. We can merge as it is.

Thanks! Regarding the failing codecov check: This seems to be caused by a different behavior of NSubstitute vs Moq. The difference is in the Register_Hooks_Should_Be_Available_At_All_Levels in the OpenFeatureHookTest - This was previously covering the exception handling in the Error hook, as Moq mocks throw an exception for methods where no behavior has been defined, while NSubstitute does not seem to do that, therefore we do not reach this catch block in the aforementioned test. Given that the purpose of this test is just to check if the Register method is available on all levels to register hooks, I think covering the exception handling of the Error hook can be part of a dedicated PR for that

These tests are the ones that miss the transition to NSubstitute:

  • Hook_Hints_May_Be_Optional
  • When_Error_Occurs_In_Before_Hook_Should_Return_Default_Value
  • When_Error_Occurs_In_After_Hook_Should_Invoke_Error_Hook

I was trying to play around but they are failing because the sequence is not respected.
I would suggest creating a new issue to migrate these three tests and I can try to fix it later.

See output:
`Expected to receive these calls in order:

Hook.Before<Boolean>(any HookContext<Boolean>, Dictionary<String, Object>)
FeatureProvider.ResolveBooleanValue(any String, any Boolean, any EvaluationContext)
Hook.After<Boolean>(any HookContext<Boolean>, any FlagEvaluationDetails<Boolean>, Dictionary<String, Object>)
Hook.Error<Boolean>(any HookContext<Boolean>, any Exception, Dictionary<String, Object>)
Hook.Finally<Boolean>(any HookContext<Boolean>, Dictionary<String, Object>)

Actually received matching calls in this order:

FeatureProvider.ResolveBooleanValue("test", True, EvaluationContext)`

@benjiro benjiro marked this pull request as ready for review August 11, 2023 12:51
@benjiro benjiro requested a review from a team as a code owner August 11, 2023 12:51
@benjiro
Copy link
Member

benjiro commented Aug 11, 2023

Thanks for your efforts @askpt, I'll look at the remaining tests tomorrow. I'll merge this or as is now

@benjiro benjiro merged commit 311987f into open-feature:main Aug 11, 2023
10 of 11 checks passed
@askpt askpt deleted the askpt/140-remove_moq branch August 11, 2023 13:14
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.

Replace Moq dependency with a different Mocking library
3 participants