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

Move startup hook tests targeting StartupHookProvider out of hosting tests #84338

Merged
merged 6 commits into from
Apr 6, 2023

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Apr 5, 2023

These tests were checking how StartupHookProvider parses out the startup hook property, handles error cases, and handles the basic valid case of loading by an assembly path or name We don't need the startup hook scenario and process launch for that validation, so they can be moved out of hosting tests:

  • Add StartupHookTests under src/tests/Loader that targets StartupHookProvider directly
  • Remove equivalent tests from HostActivation.Tests
  • Remove associated test asset projects (no longer need to be restored during build and build/copied during test run)

Running locally (Windows):

  • Host StartupHooks tests went from 1.9min to 9.5s on Release
  • New tests took 0.14s on Release coreclr (4.5s on Debug)

Replaced tests:

Old New
Muxer_activation_of_StartupHook_Succeeds ValidHookPath
Muxer_activation_of_Multiple_StartupHooks_Succeeds MultipleValidHooksAndSeparators
Muxer_activation_of_StartupHook_VariableVariants MultipleValidHooksAndSeparators
Muxer_activation_of_StartupHook_With_Invalid_Simple_Name_Fails InvalidSimpleAssemblyName
Muxer_activation_of_StartupHook_With_Missing_Assembly_Fails MissingAssembly
Muxer_activation_of_StartupHook_WithSimpleAssemblyName_Succeeds ValidHookName
Muxer_activation_of_Missing_StartupHook_Assembly_Fails MissingAssembly
Muxer_activation_of_Invalid_StartupHook_Assembly_Fails InvalidAssembly
Muxer_activation_of_Missing_StartupHook_Type_Fails MissingStartupHookType
Muxer_activation_of_StartupHook_With_Missing_Method_Fails MissingInitializeMethod
Muxer_activation_of_StartupHook_With_Incorrect_Method_Signature_Fails IncorrectInitializeSignature

The one thing the new tests don't check is that a startup hook failure means an app run failure. We do still have a test on the host side that expects failure - I think that is reasonable enough to not also need the same check for all the other possible failure points.

Resolves #77805
Contributes to #77807

@ghost
Copy link

ghost commented Apr 5, 2023

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

These tests were checking how StartupHookProvider parses out the startup hook property, handles error cases, and handles the basic valid case of loading by an assembly path or name We don't need the startup hook scenario and process launch for that validation, so they can be moved out of hosting tests:

  • Add StartupHookTests under src/tests/Loader that targets StartupHookProvider directly
  • Remove equivalent tests from HostActivation.Tests
  • Remove associated test asset projects (no longer need to be restored during build and build/copied during test run)

Running locally (Windows):

  • Host StartupHooks tests went from 1.9min to 9.5s on Release
  • New tests took 0.14s on Release coreclr (4.5s on Debug)

Replaced tests:

Old New
Muxer_activation_of_StartupHook_Succeeds ValidHookPath
Muxer_activation_of_Multiple_StartupHooks_Succeeds MultipleValidHooksAndSeparators
Muxer_activation_of_StartupHook_VariableVariants MultipleValidHooksAndSeparators
Muxer_activation_of_StartupHook_With_Invalid_Simple_Name_Fails InvalidSimpleAssemblyName
Muxer_activation_of_StartupHook_With_Missing_Assembly_Fails MissingAssembly
Muxer_activation_of_StartupHook_WithSimpleAssemblyName_Succeeds ValidHookName
Muxer_activation_of_Missing_StartupHook_Assembly_Fails MissingAssembly
Muxer_activation_of_Invalid_StartupHook_Assembly_Fails InvalidAssembly
Muxer_activation_of_Missing_StartupHook_Type_Fails MissingStartupHookType
Muxer_activation_of_StartupHook_With_Missing_Method_Fails MissingInitializeMethod
Muxer_activation_of_StartupHook_With_Incorrect_Method_Signature_Fails IncorrectInitializeSignature

The one thing the new tests don't check is that a startup hook failure means an app run failure. We do still have a test on the host side that expects failure - I think that is reasonable enough to not also need the same check for all the other possible failure points.

Resolves #77805

Author: elinor-fung
Assignees: -
Labels:

area-Host

Milestone: -

Copy link
Member

@jkoritzinsky jkoritzinsky 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 using the new [Fact] style tests! Do we want to mark this test suite as RequiresProcessIsolation, or is that not necessary?

@elinor-fung elinor-fung force-pushed the moveStartupHookTests branch from f4b4fee to 4cb2cae Compare April 5, 2023 02:06
@elinor-fung
Copy link
Member Author

Do we want to mark this test suite as RequiresProcessIsolation, or is that not necessary?

Does the merged runner execute tests from different assemblies in parallel? I don't recall. If so, I guess technically it should be, since other things could (although unlikely) modify the STARTUP_HOOKS app context data while test from this class are running. Otherwise, I don't think it should be necessary (but I should make it IDisposable so it cleans up whatever STARTUP_HOOKS value it set).

@jkoritzinsky
Copy link
Member

It doesn't execute tests from different assemblies in parallel today, but it may do so in the future.

@elinor-fung
Copy link
Member Author

Actually, the success-case tests do also assume that there isn't some already loaded assembly with the same name. I'll make it RequiresProcessIsolation. Thanks!

@runfoapp runfoapp bot mentioned this pull request Apr 5, 2023
@elinor-fung elinor-fung merged commit 47aab33 into dotnet:main Apr 6, 2023
@elinor-fung elinor-fung deleted the moveStartupHookTests branch April 6, 2023 22:36
@ghost ghost locked as resolved and limited conversation to collaborators May 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move startup hook tests targeting StartupHookProvider out of hosting tests
3 participants