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

Improve test fixtures and related code #3637

Merged
merged 6 commits into from
Oct 4, 2020

Conversation

Nirmal4G
Copy link
Contributor

@Nirmal4G Nirmal4G commented Sep 3, 2020

Bug

Fixes: NuGet/Home#9996
Regression: No

Fix

Details: These are some of the improvements separated from the PR #3270

  • Consolidate Restore/Build/Pack methods in Tests
    • Test Fixture now has similar API surface across Restore/Build/Pack commands
  • Fix 'dotnet pack' tests when testing under VS IDE
    • The NuGet Pack artifacts aren't being copied to the test SDK layout, when building/testing under VS IDE locally and not on CI/CLI.
  • Rename 'OutputPath' to 'ProjectExtensionsPath' in 'SimpleTestProjectContext'
    • The 'OutputPath' property under 'SimpleTestProjectContext' is an alias of 'MSBuildProjectExtensionsPath', but there is an 'OutputPath' property already in MSBuild project system.
  • Refactor 'CreateDotnetToolProject' method & calls in 'DotNetToolTests'
    • Rename 'source' param to 'packageSource'
    • Make 'packageSource' param to accept null
    • Fix template's 'MSBuildProjectExtensionsPath' with explicit Sdk import
    • Remove redundant parameter labels on 'CreateDotnetToolProject' in 'DotNetToolTests'
  • Miscellaneous refactoring and whitespace fixes

Testing/Validation

Tests Added: No
Reason for not adding tests: These are tests themselves
Validation: As long as CI succeeds

@Nirmal4G Nirmal4G marked this pull request as ready for review September 3, 2020 18:35
@Nirmal4G Nirmal4G requested a review from a team as a code owner September 3, 2020 18:35
@zivkan zivkan added Approved for CI Community PRs created by someone not in the NuGet team labels Sep 3, 2020
Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

just making sure no one accidentally merges this before an issue is linked.

@zivkan
Copy link
Member

zivkan commented Sep 4, 2020

The test that failed is Dotnet.Integration.Test.PackCommandTests.PackCommand_PackConsoleAppWithRID_NupkgValid, with the error message:

The output .nupkg is not in the default place
Expected: True
Actual:   False

and stack trace

at Dotnet.Integration.Test.PackCommandTests.PackCommand_PackConsoleAppWithRID_NupkgValid(Boolean includeSymbols) in C:\A\1\106\s\test\NuGet.Core.FuncTests\Dotnet.Integration.Test\PackCommandTests.cs:line 194

I see you modified this test in your PR, plus I ran the CI build twice and it failed both times, so I think it's unlikely to be flaky.

Dotnet.Integration.Tests should be runnable from Visual Studio's Test Explorer (or I guess from the command line, although I've never tried debugging a test in VSCode or anything other than full VS). So, I think you should be able to repro the test failure locally.

@Nirmal4G Nirmal4G force-pushed the hotfix/improve-test-fixtures branch from dc401d8 to 7e90506 Compare September 4, 2020 11:29
@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Sep 4, 2020

I see you modified this test in your PR

It was a rebase error, I put a wrong patch on the wrong branch! My bad!! Sorry.

Dotnet.Integration.Tests should be runnable from Visual Studio's Test Explorer

This is how I'm testing the code. I tried VSCode at first, not a good experience. C# Extension (Omnisharp) fails to load some projects everytime and everytime those failed projects are different which makes it hard to pinpoint the source of the issue. So, I gave up and went back to VS IDE!

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Consolidate Restore/Build/Pack methods in Tests
Test Fixture now has similar API surface across Restore/Build/Pack commands

That's reasonable, thanks.

Fix 'dotnet pack' tests when testing under VS IDE
The NuGet Pack artifacts aren't being copied to the test SDK layout, when building/testing under VS IDE locally and not on CI/CLI.

I'm not sure what doesn't work in this situation. I did a quick test locally and we do copy the assets. We even have an optimization to determine whether we need to copy the ilmerged or non-ilmerged pack dll.

Rename 'OutputPath' to 'ProjectExtensionsPath' in 'SimpleTestProjectContext'
The 'OutputPath' property under 'SimpleTestProjectContext' is an alias of 'MSBuildProjectExtensionsPath', but there is an 'OutputPath' property already in MSBuild project system.

These changes are in a different PR, so I think that's ok, just update the description :)

Refactor 'CreateDotnetToolProject' method & calls in 'DotNetToolTests'
Rename 'source' param to 'packageSource'
Make 'packageSource' param to accept null
Fix template's 'MSBuildProjectExtensionsPath' with explicit Sdk import
Remove redundant parameter labels on 'CreateDotnetToolProject' in 'DotNetToolTests'

Same here, not sure I see these changes.

@Nirmal4G Nirmal4G force-pushed the hotfix/improve-test-fixtures branch from 0974407 to d3ebf5b Compare September 21, 2020 18:41
@nkolev92
Copy link
Member

Hey @Nirmal4G Is it ready for review again?

@Nirmal4G
Copy link
Contributor Author

Yes, go ahead. 👍

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

LGTM

@Nirmal4G Nirmal4G force-pushed the hotfix/improve-test-fixtures branch 4 times, most recently from 8164110 to 9d54fa7 Compare September 28, 2020 19:12
@Nirmal4G Nirmal4G force-pushed the hotfix/improve-test-fixtures branch from 9d54fa7 to b3123fd Compare October 1, 2020 21:12
Test Fixture now has similar API surface across Restore/Build/Pack commands
The NuGet Pack artifacts aren't being copied to the test SDK layout,
when building/testing under VS IDE locally and not on CI/CLI.
…ontext'

The 'OutputPath' property under 'SimpleTestProjectContext' is an alias of 'MSBuildProjectExtensionsPath',
but there is an 'OutputPath' property already in MSBuild project system.

So, Rename 'OutputPath' to 'ProjectExtensionsPath' to better reflect it's intent.
Rename 'source' param to 'packageSources'
Make 'packageSources' param to accept null
Fix template's 'MSBuildProjectExtensionsPath' with explicit Sdk import
Remove redundant parameter labels on 'CreateDotnetToolProject' in 'DotNetToolTests'
cosmetic refactors that are leftover from this patch tree
Only files in this patch have been trimmed for trailing whitespace
@Nirmal4G Nirmal4G force-pushed the hotfix/improve-test-fixtures branch from b3123fd to ce3ef70 Compare October 3, 2020 10:45
Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

@Nirmal4G thanks again for the reminder, and sorry for again being slow. Another deadline has passed, so pressure to finish high priority work is over and I want to spend more time making sure community contributions such as yours get through.

@zivkan zivkan merged commit 97311b5 into NuGet:dev Oct 4, 2020
@Nirmal4G Nirmal4G deleted the hotfix/improve-test-fixtures branch October 4, 2020 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix 'dotnet pack' tests when testing under VS IDE
4 participants