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

Introduce OrchardCore.Testing APIs #13018

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Introduce OrchardCore.Testing APIs #13018

wants to merge 38 commits into from

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Dec 31, 2022

This is one of the PRs that I'm working on testing part of Orchard Core, the plan is to provide an essential testing APIs infrastructure for creating unit tests easily for OC APIs and modules

For reference #11865 (comment)
Fixes #11865.

@hishamco hishamco mentioned this pull request Dec 31, 2022
# Conflicts:
#	src/OrchardCore/OrchardCore.Testing/Apis/Security/PermissionContextAuthorizationHandler.cs
#	src/OrchardCore/OrchardCore.Testing/Data/TablePrefixGenerator.cs
#	src/OrchardCore/OrchardCore.Testing/Extensions/HttpContentExtensions.cs
#	test/OrchardCore.Testing.Tests/Data/TablePrefixGeneratorTests.cs
#	test/OrchardCore.Testing.Tests/Security/PermissionHandlerTests.cs
#	test/OrchardCore.Tests/Apis/ContentManagement/ContentApiController/BlogPostApiControllerTests.cs
#	test/OrchardCore.Tests/Apis/ContentManagement/DeploymentPlans/BlogPostCreateDeploymentPlanTests.cs
#	test/OrchardCore.Tests/Apis/ContentManagement/DeploymentPlans/BlogPostUpdateDeploymentPlanTests.cs
#	test/OrchardCore.Tests/Apis/Context/BlogPostApiControllerContext.cs
#	test/OrchardCore.Tests/Apis/Context/Extensions/HttpRequestExtensions.cs
#	test/OrchardCore.Tests/Apis/Context/MvcTestFixture.cs
#	test/OrchardCore.Tests/Apis/Context/SiteContext.cs
#	test/OrchardCore.Tests/Apis/Context/SiteStartup.cs
#	test/OrchardCore.Tests/Apis/Context/TestRecipeHarvester.cs
#	test/OrchardCore.Tests/Apis/GraphQL/ValidationRules/RequiresPermissionValidationRuleTests.cs
#	test/OrchardCore.Tests/Apis/Lucene/LuceneQueryTests.cs
#	test/OrchardCore.Tests/DisplayManagement/Decriptors/DefaultShapeTableManagerTests.cs
#	test/OrchardCore.Tests/DisplayManagement/DefaultDisplayManagerTests.cs
#	test/OrchardCore.Tests/DisplayManagement/ShapeFactoryTests.cs
#	test/OrchardCore.Tests/DisplayManagement/ShapeHelperTests.cs
#	test/OrchardCore.Tests/DisplayManagement/ShapeSerializerTests.cs
#	test/OrchardCore.Tests/Email/EmailTests.cs
#	test/OrchardCore.Tests/Extensions/ExtensionManagerTests.cs
#	test/OrchardCore.Tests/Localization/PortableObjectStringLocalizerFactoryTests.cs
#	test/OrchardCore.Tests/Localization/PortableObjectStringLocalizerTests.cs
#	test/OrchardCore.Tests/Modules/OrchardCore.OpenId/OpenIdServerDeploymentSourceTests.cs
#	test/OrchardCore.Tests/Modules/OrchardCore.Roles/RolesPermissionsHandlerTests.cs
#	test/OrchardCore.Tests/OrchardCore.Users/UserValidatorTests.cs
#	test/OrchardCore.Tests/OrchardCore.Users/UsersMockHelper.cs
#	test/OrchardCore.Tests/ResourceManagement/ResourceDefinitionTests.cs
#	test/OrchardCore.Tests/ResourceManagement/ResourceManagerTests.cs
#	test/OrchardCore.Tests/Routing/AutorouteEntriesTests.cs
#	test/OrchardCore.Tests/Security/PermissionHandlerHelper.cs
#	test/OrchardCore.Tests/Shell/ShellContainerFactoryTests.cs
#	test/OrchardCore.Tests/Stubs/MemoryFileBuilder.cs
#	test/OrchardCore.Tests/Stubs/StubExtensionManager.cs
#	test/OrchardCore.Tests/Stubs/StubHostingEnvironment.cs
#	test/OrchardCore.Tests/Stubs/StubPoFileLocationProvider.cs
#	test/OrchardCore.Tests/Workflows/WorkflowManagerTests.cs
Copy link

This pull request has merge conflicts. Please resolve those before requesting a review.

@Piedone
Copy link
Member

Piedone commented Mar 19, 2024

Do you want to get back to this anytime soon, @hishamco? Starting with addressing #13018 (comment).

Otherwise, I welcome refactoring the tests and providing some common packages to make it easier for community members to write tests for their OC modules. (A bit of that we also address with the Lombiq Testing Toolbox but this would be better as part of OC itself.) I didn't look deeply into the code under this PR yet.

Also, you may want to look into AutoMocker: I briefly checked out the code and I see a lot of manual dependency resolution with mocks, while this library takes care of that.

@hishamco
Copy link
Member Author

I am delaying this because I've been working on Playwright and OC integrations last few weeks, hope to have a demo ASAP. Regarding I will try AutoMock and MockQueryable.Moq because they are so useful

@MichaelPetrinolis
Copy link
Contributor

Hello @hishamco, will this PR be a part of OC 2?

@hishamco
Copy link
Member Author

I will try to resolve the changes, there're too much :)

@MichaelPetrinolis
Copy link
Contributor

@hishamco Perfect! If I can help, please let me know.

@hishamco
Copy link
Member Author

@MichaelPetrinolis if you fix the conflict I will be thankful, coz I'm working on other PRs and issues

@MichaelPetrinolis
Copy link
Contributor

@hishamco weekend is over but i will try to get time for it this week.

@MichaelPetrinolis
Copy link
Contributor

@hishamco why is preferable to use the UseCulture attribute in tests instead of using the CultureScope ?

@hishamco
Copy link
Member Author

We could but I did it as a wrapper, attributes are easier to use

@MichaelPetrinolis
Copy link
Contributor

@hishamco I merged main into a new branch. I have some failed tests, I believe due to HttpContextAccessor not set and PermissionsContext. You can check it out here https://github.com/MichaelPetrinolis/OrchardCore/tree/merge_testing

@MichaelPetrinolis
Copy link
Contributor

@Piedone @hishamco I think that we should try to merge this PR (when is complete) for v2.0.0
When it gets outdated again, it will be time-consuming bringing up all the changes.
There is a number difference in tests, seven tests were moved to another test project

@hishamco
Copy link
Member Author

You're right @MichaelPetrinolis could you please commit directly here, I will spend time today to make it ready, also adding your commits here is useful to have a credit for you :)

Hope to get this finished before the end of next week

@MichaelPetrinolis
Copy link
Contributor

@hishamco I don't have permission to directly push to your branch

@hishamco
Copy link
Member Author

hishamco commented May 31, 2024

You're not in OC team?!!

@MichaelPetrinolis
Copy link
Contributor

don't know, I usually send PRs from my fork

@hishamco
Copy link
Member Author

You're not in OC team?!!

Seems you're not because I didn't see the member label beside your comment

So I will look to your changes, and see how it goes

@MichaelPetrinolis
Copy link
Contributor

Ι can send a PR to your branch, if you can perform a merge commit

@MichaelPetrinolis
Copy link
Contributor

the PR is ready, there are some tests that fail, I think because the HttpContextAccessor is not properly set. Also, I did not include the readme.md in the nav configuration and the check fails.

@hishamco
Copy link
Member Author

I did not include the readme.md in the nav configuration and the check fails.

Is it there before?

@MichaelPetrinolis
Copy link
Contributor

I did a quick check, and it was not. Obviously, we should add it.

@hishamco
Copy link
Member Author

Look I will check if I can merge your commits locally

@Piedone
Copy link
Member

Piedone commented May 31, 2024

Before anything else, what I told previously needs to be addressed:

Also, you may want to look into AutoMocker: I briefly checked out the code and I see a lot of manual dependency resolution with mocks, while this library takes care of that.

@MichaelPetrinolis
Copy link
Contributor

Before anything else, what I told previously needs to be addressed:

Also, you may want to look into AutoMocker: I briefly checked out the code and I see a lot of manual dependency resolution with mocks, while this library takes care of that.

Maybe we should extract and merge the OrchardCore.Testing library from OrchardCore.Tests and in another issue refactor the tests to use AutoMocker.

@Piedone
Copy link
Member

Piedone commented May 31, 2024

This PR adds a lot of new stuff, which isn't needed with AutoMocker. So I don't see why we'd add these, and then remove them in a second step, instead of not adding them in the first place.

@MichaelPetrinolis
Copy link
Contributor

Hello @hishamco, is there anything else I can do to contribute to this PR?

@hishamco
Copy link
Member Author

Sorry @MichaelPetrinolis for late I will try to make this as a top of my ToDo list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OrchardCore Testing
4 participants