Skip to content

Support passing arguments to DynamicData methods #5892

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

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

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jun 27, 2025

Fixes #1510

  • Revisit DynamicDataShouldBeValidAnalyzer

@microsoft-github-policy-service microsoft-github-policy-service bot added the Area: MSTest Issues with MSTest that are not specific to more refined area (e.g. analyzers or assertions) label Jun 27, 2025
@Youssef1313 Youssef1313 marked this pull request as ready for review June 27, 2025 10:37
@Youssef1313 Youssef1313 requested a review from Evangelink July 7, 2025 13:00
@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2025

Codecov Report

Attention: Patch coverage is 86.44068% with 8 lines in your changes missing coverage. Please review.

Project coverage is 73.31%. Comparing base (cab3e7f) to head (594f3fb).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ork/Attributes/DataSource/DynamicDataOperations.cs 84.84% 5 Missing ⚠️
...work/Attributes/DataSource/DynamicDataAttribute.cs 83.33% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5892      +/-   ##
==========================================
- Coverage   73.32%   73.31%   -0.01%     
==========================================
  Files         613      613              
  Lines       37574    37636      +62     
==========================================
+ Hits        27552    27594      +42     
- Misses      10022    10042      +20     
Flag Coverage Δ
Debug 73.31% <86.44%> (+0.36%) ⬆️
integration 73.31% <86.44%> (+0.36%) ⬆️
production 73.31% <86.44%> (+0.36%) ⬆️
unit 73.31% <86.44%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...Test.Analyzers/DynamicDataShouldBeValidAnalyzer.cs 93.99% <100.00%> (+0.15%) ⬆️
...work/Attributes/DataSource/DynamicDataAttribute.cs 87.95% <83.33%> (-12.05%) ⬇️
...ork/Attributes/DataSource/DynamicDataOperations.cs 89.76% <84.84%> (+1.19%) ⬆️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

=> Assert.IsInRange(4, 6, a);

[TestMethod]
[DynamicData(nameof(GetData2), [new int[] { 4, 5, 6 }])]
Copy link
Member

Choose a reason for hiding this comment

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

it might be good to add tests for the multiple parameter case and also for object[] parameter case

Copy link
Member

Choose a reason for hiding this comment

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

Looking at DataRow tests, we probably want:

  • 1 value
  • few values
  • tuple
  • string[]
  • object[]

And a case where the data member declares parameters with params (whether or not we support it).

It's probably also good if we have an acceptance test asserting logs + test didn't run for invalid input cases (too few, too many, invalid type args).

Copy link
Member Author

Choose a reason for hiding this comment

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

@Evangelink Just to be clear, do we want to make it special case params (ParamArrayAttribute)? Similar to here? Or just treat it as object[], in that case it's no different from any other parameter? Or probably block the usage of params until we make a decision?

Copy link
Member

Choose a reason for hiding this comment

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

Whatever is easier. I am not a big fan of params but I know some people are using it. Given this is a new feature, we are quite free and I am perfectly fine to wait for feedback. On the other hand, if that's "easy" to do, it's probably better to have similarity in all places.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll disallow params for now.

using System.Collections.Generic;
using Microsoft.VisualStudio.TestTools.UnitTesting;
[TestClass]
public class MyTestClass
Copy link
Member

Choose a reason for hiding this comment

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

is there a test for a failing scenario? where we do pass in the incorrect number of parameters?

@Youssef1313 Youssef1313 force-pushed the dev/ygerges/parameterize-dynamicdata branch from 6c339cf to 90e0908 Compare July 11, 2025 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: MSTest Issues with MSTest that are not specific to more refined area (e.g. analyzers or assertions)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for passing in arguments for methods used to store dynamic data
4 participants