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

Update MSTest.Test* packages from 3.6.4 to 3.7.0, follow best practices #848

Merged
merged 20 commits into from
Jan 2, 2025

Conversation

DaveTryon
Copy link
Contributor

@DaveTryon DaveTryon commented Dec 31, 2024

Dependabot tried to update MSTest.TestFramework from 3.6.4 to 3.7.0 in #839, and MSTest.TestAdapter in #841. These PR's both failed, as the 2 pieces need to move together for the tests to work correctly. In addition, the newer version of MSTest.TestFramework adds several new warnings, and since our builds treat warnings as errors, the build breaks.

This PR touches a lot of files, but since these packages are all test-specific, so are the code changes. It was done as a series of commits, and reviewing each commit in sequence will probably simplify the review process. Commits are as follows:

Commit Description/Coment
1 Unchanged from #839
2 Suppress new warnings
3 Unchanged from #841 -- this allowed the tests to run
4 Added a suppression that was flagged once both packages were updated
5 MSTEST0005 We were using TestContext improperly, fix it
6 MSTEST0006 ExpectedException is no longer considered a best practice, so I changed them to ThrowsException or ThrowsExceptionAsync. Most of these changes were tool-driven, but some were manual and exposed some dead code in the unit tests
7 MSTEST0017 Assert.AreEqual wants the expected parameter first--there were many places in the code where we had the test parameter first. These changes were largely tool-driven
8 MSTEST0024 was fixed by 5, so the suppression can go away
9 MSTEST0034 This just adds a parameter to an attribute decoration
10 MSTEST0037 Assert.AreEqual, Assert.IsNull, and Assert.IsNotNull are all preferred over Assert.IsTrue becuase they are more descriptive. These changes were largely tool-driven.
11 I noticed a few more places where the parameters of Assert.AreEqual were in the wrong order, but the analyzer failed to catch the error. These changes were manual.
12 MSTEST0026 Null propagation in asserts is discouraged
13 MSTEST0032 Non-nullable types don't need to be asserted as non-null. Note that this undoes some of the changes from 12
14 JSON002 is no longer showing up as an error, so I removed the suppression
15 Remove a couple of using blocks that were accidentally added along the way
16 Restore a check was dropped in 6
17 Fix broken tests on macOS, add local JSON002 (only seems to impact Visual Studio, not command line)

Note: When running from inside Visual Studio, the net472 projects get skipped. They run correctly from the command line, so any potential regressions should get caught in the pipelines. The net80 versions of the tests work as expected in both Visual Studio and from the command line. I opened microsoft/testfx#4487 to report that issue. It was resolved as a dupe of microsoft/testfx#4426

@DaveTryon DaveTryon requested a review from a team as a code owner December 31, 2024 20:36
@codecov-commenter
Copy link

codecov-commenter commented Dec 31, 2024

Codecov Report

Attention: Patch coverage is 0% with 128 lines in your changes missing coverage. Please review.

Project coverage is 46.50%. Comparing base (9ef64aa) to head (10b76c4).

Files with missing lines Patch % Lines
...bom.Api.Tests/PackageDetails/RubyGemsUtilsTests.cs 0.00% 16 Missing ⚠️
...s/Config/ConfigurationBuilderTestsForValidation.cs 0.00% 14 Missing ⚠️
...s/Config/ConfigurationBuilderTestsForGeneration.cs 0.00% 13 Missing ⚠️
...bom.Api.Tests/Executors/FileListEnumeratorTests.cs 0.00% 8 Missing ⚠️
...bom.Api.Tests/Entities/FileValidationResultTest.cs 0.00% 5 Missing ⚠️
...t.Sbom.Api.Tests/Executors/DirectoryWalkerTests.cs 0.00% 5 Missing ⚠️
...ft.Sbom.Api.Tests/Executors/PackagesWalkerTests.cs 0.00% 5 Missing ⚠️
.../Executors/ComponentToPackageInfoConverterTests.cs 0.00% 4 Missing ⚠️
...Utils/ComponentDetectionCliArgumentBuilderTests.cs 0.00% 4 Missing ⚠️
...Tests/Utils/ComponentDetectorCachedExecutorTest.cs 0.00% 4 Missing ⚠️
... and 23 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #848      +/-   ##
==========================================
+ Coverage   46.47%   46.50%   +0.02%     
==========================================
  Files         355      355              
  Lines       14151    14142       -9     
  Branches     1163     1161       -2     
==========================================
  Hits         6577     6577              
+ Misses       7026     7017       -9     
  Partials      548      548              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@alisonlomaka alisonlomaka left a comment

Choose a reason for hiding this comment

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

I saw in your comments that you used tooling to assist some of these refactors - might make an interesting short and sweet tech talk/demo for the team.

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

I would also strongly encourage you to enable test parallelization either from runsettings or using the folowing assembly level attribute [assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)].

Finally, it'd be good to also consider moving to MSTest runner, which is faster, more ligtweight and the recommended way to run your tests.

@DaveTryon
Copy link
Contributor Author

I would also strongly encourage you to enable test parallelization either from runsettings or using the folowing assembly level attribute [assembly: Parallelize(Scope = ExecutionScope.MethodLevel, Workers = 0)].

Finally, it'd be good to also consider moving to MSTest runner, which is faster, more ligtweight and the recommended way to run your tests.

@Evangelink, this entire PR was unexpected and frankly work that we hadn't scheduled. It's bloomed to 43 files, which is way too big for a single PR. Trying to add parallelization to this PR seems like it would only lead to pain and more churn. Please feel free to open an issue suggesting the parallelization feature and/or moving to MSTest runner, but I'll honestly say that it's unlikely to find much traction unless there's a compelling reason--our tests are fast enough that the inner dev loop won't be significantly improved by shaving a few seconds from the run time.

@DaveTryon DaveTryon merged commit 9ed398b into main Jan 2, 2025
7 checks passed
@DaveTryon DaveTryon deleted the DaveTryon/update-test-packages branch January 2, 2025 20:58
DaveTryon added a commit that referenced this pull request Jan 6, 2025
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.

5 participants