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

Refactor and improve MLL tests #66332

Merged
merged 4 commits into from
Mar 11, 2022
Merged

Conversation

vitek-karas
Copy link
Member

  • Remove duplicated code - I know that tests should be descriptive, but repeating 100 times that we want to capture output doesn't feel necessary.

  • For some of them move more stuff into the shared test state - this improves perf as we avoid repeating the setup (copying files around) for each test case, we do it once for the entire class
    I also changed some of the tests to "Theory" as it's easier to read that way.

    • SDKLookup.cs - moved most of the state in to the shared state to speed up the tests.
  • Adding more cases into the "theory" tests

    • Most notably for the framework resolution I added variations on the TFM (which will be needed when we implement disable of MLL)
  • Adding new tests mostly around "list runtimes" (various forms), "list sdks" (various forms) and errors (which also list runtimes or sdks)

  • Ported all of the MultiLevelLookupFramework tests over to the FrameworkResolution and DependencyResolutions suites which have a more robust test infra and can run the same tests much faster. Along the way I added lot more variations on top of the existing tests:

    • PerAssemblyVersionResolutionMultipleFrameworks.cs - this is actually not an MLL test, but I moved it to the new infra to make it much faster
    • MultipleHives.cs - MLL framework resolution tests

For SDK resolution I kept the MultiLevelSDKLookup.cs just removed code duplication and added new variants.

For the core reviewers: I promise I didn't remove any single test case in spirit with these exceptions:

  • We had tests which validated that framework resolution is not affected by frameworks in current directory and also by frameworks in the user's directory. I left some basic test for the current directory check, but I completely removed the user's directory variant as the product simply doesn't have any code around that anymore.

* Remove duplicated code - I know that tests should be descriptive, but repeating 100 times that we want to capture output doesn't feel necessary.
* For some of them move more stuff into the shared test state - this improves perf as we avoid repeating the setup (copying files around) for each test case, we do it once for the entire class
I also changed some of the tests to "Theory" as it's easier to read that way.
  * `SDKLookup.cs` - moved most of the state in to the shared state to speed up the tests.

* Adding more cases into the "theory" tests
  * Most notably for the framework resolution I added variations on the TFM (which will be needed when we implement disable of MLL)
* Adding new tests mostly around "list runtimes" (various forms), "list sdks" (various forms) and errors (which also list runtimes or sdks)

* Ported all of the `MultiLevelLookupFramework` tests over to the `FrameworkResolution` and `DependencyResolutions` suites which have a more robust test infra and can run the same tests much faster. Along the way I added lot more variations on top of the existing tests:
  * `PerAssemblyVersionResolutionMultipleFrameworks.cs` - this is actually not an MLL test, but I moved it to the new infra to make it much faster
  * `MultipleHives.cs` - MLL framework resolution tests

For SDK resolution I kept the `MultiLevelSDKLookup.cs` just removed code duplication and added new variants.

For the core reviewers: I promise I didn't remove any single test case in spirit with these exceptions:

* We had tests which validated that framework resolution is not affected by frameworks in current directory and also by frameworks in the user's directory. I left some basic test for the current directory check, but I completely removed the user's directory variant as the product simply doesn't have any code around that anymore.
@vitek-karas vitek-karas requested a review from elinor-fung March 8, 2022 13:26
@vitek-karas vitek-karas self-assigned this Mar 8, 2022
@ghost
Copy link

ghost commented Mar 8, 2022

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

Issue Details
  • Remove duplicated code - I know that tests should be descriptive, but repeating 100 times that we want to capture output doesn't feel necessary.

  • For some of them move more stuff into the shared test state - this improves perf as we avoid repeating the setup (copying files around) for each test case, we do it once for the entire class
    I also changed some of the tests to "Theory" as it's easier to read that way.

    • SDKLookup.cs - moved most of the state in to the shared state to speed up the tests.
  • Adding more cases into the "theory" tests

    • Most notably for the framework resolution I added variations on the TFM (which will be needed when we implement disable of MLL)
  • Adding new tests mostly around "list runtimes" (various forms), "list sdks" (various forms) and errors (which also list runtimes or sdks)

  • Ported all of the MultiLevelLookupFramework tests over to the FrameworkResolution and DependencyResolutions suites which have a more robust test infra and can run the same tests much faster. Along the way I added lot more variations on top of the existing tests:

    • PerAssemblyVersionResolutionMultipleFrameworks.cs - this is actually not an MLL test, but I moved it to the new infra to make it much faster
    • MultipleHives.cs - MLL framework resolution tests

For SDK resolution I kept the MultiLevelSDKLookup.cs just removed code duplication and added new variants.

For the core reviewers: I promise I didn't remove any single test case in spirit with these exceptions:

  • We had tests which validated that framework resolution is not affected by frameworks in current directory and also by frameworks in the user's directory. I left some basic test for the current directory check, but I completely removed the user's directory variant as the product simply doesn't have any code around that anymore.
Author: vitek-karas
Assignees: vitek-karas
Labels:

area-Host

Milestone: -

@elinor-fung
Copy link
Member

completely removed the user's directory variant as the product simply doesn't have any code around that anymore

Could we do this for the SDK lookup tests too? I don't recall ever seeing anything around user directory for SDK resolution either.

The product simply doesn't have any knoweldge about user directory SDK lookup, so there's little value in testing for it (even though the test expects it to not have any impact).
@vitek-karas
Copy link
Member Author

Good point - I removed the "user directory" parts from SDK resolution tests as well.

vitek-karas and others added 2 commits March 10, 2022 03:09
Co-authored-by: Elinor Fung <elfung@microsoft.com>
@vitek-karas
Copy link
Member Author

The test failures are:
#66174
#65954

@vitek-karas vitek-karas merged commit f82fe1d into dotnet:main Mar 11, 2022
@vitek-karas vitek-karas deleted the ImproveMLLTests branch March 11, 2022 08:34
@ghost ghost locked as resolved and limited conversation to collaborators Apr 10, 2022
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.

2 participants