search

SpecFlowOSS / SpecFlow

#1 .NET BDD Framework. SpecFlow automates your testing & works with your existing code. Find Bugs before they happen. Behavior Driven Development helps developers, testers, and business representatives to get a better understanding of their collaboration
https://www.specflow.org/
Other
2.24k stars 754 forks source link

Bug: Value retriever based on column names not retrieving value #2205

Closed timvandenhof closed 3 years ago

timvandenhof commented 3 years ago

SpecFlow Version:

Used Test Runner

Version number:

Project Format of the SpecFlow project

.feature.cs files are generated using

Does not apply, see commit.

Visual Studio Version

Visual Studio 2019 - 16.8.0

'Enable SpecFlowSingleFileGenerator Custom Tool' option in Visual Studio extension settings

Does not apply.

Are the latest Visual Studio updates installed?

.NET Framework:

Test Execution Method:

<SpecFlow> Section in app.config or content of specflow.json

Does not apply, see commit.

Issue Description

When using a custom IValueRetriever to map a specific column names to a specific property, Retrieve is not being called while CanRetrieve returned true. This was unexpected to me.

Diving deeper into the Specflow sources to find test cases I noticed that there were testcases, but non of them were covering this scenario. I added a test case in this commit:

Is this a feature that should be working in the way I suspect, or is this indeed a bug?

If this is considered a bug: the issue is caused in file TEHelper.cs by GetMembersThatNeedToBeSet, which is checking the CanResolve but also requiring the column name to match exactly or on an alias. Which won't be the case in this scenario, resulting in Resolve not being called.

Potential workaround: use an TableAliasesAttribute, if possible (eg. not instantiating third party object.)

Steps to Reproduce

  1. Create an IValueRetriever implementation.
  2. Use specific column names to map them to a complex type. Check for colum and type match in CanRetrieve.
  3. The implementation of Retrieve is irrelevant, as it is not being called.
  4. Create a feature file (or unittest) to reproduce the issue, where the colum name is used. See repro project for example.

Example:

public class ParentItem
{
    public string Name { get; set; }

    public ChildItem DutchItem { get; set; }

    public ChildItem EnglishItem { get; set; }
}

public class ChildItem
{
    public string ItemName { get; set; }

    public string Language { get; set; }
}

/// <summary>
/// Retrieves a ChildItem-objects for column's "Dutch name" and "English name" and maps them
/// to the corresponding "DutchItem" and "EnglishItem" properties of a ParentItem-object.
/// </summary>
public class ChildItemValueRetriever : IValueRetriever
{
    private static readonly IEnumerable<Type> TypesForWhichIRetrieveValues = new Type[] { typeof(ChildItem) };

    private static readonly IDictionary<string, string> ColumnNamesForWhichIRetrieveValues =
        new Dictionary<string, string>
        {
            { "Dutch name", "nl-NL" },
            { "English name", "en-US" }
        };

    public bool CanRetrieve(KeyValuePair<string, string> keyValuePair, Type targetType, Type type)
    {
        return TypesForWhichIRetrieveValues.Contains(type) 
            && ColumnNamesForWhichIRetrieveValues.ContainsKey(keyValuePair.Key);
    }

    public object Retrieve(KeyValuePair<string, string> keyValuePair, Type targetType, Type propertyType)
    {
        return Parse(keyValuePair.Key, keyValuePair.Value);
    }

    public static ChildItem Parse(string columnName, string columnValue)
    {
        return new ChildItem
        {
            ItemName = columnValue,
            Language = ColumnNamesForWhichIRetrieveValues[columnName]
        };
    }
}

Repro Project

See the following commit on my SpecFlow fork. It is based on the example above. https://github.com/timvandenhof/SpecFlow/commit/dad342db4a67138714c8b8f598a2e8f338d4b6c6

SabotageAndi commented 3 years ago

@timvandenhof Could you send this failing tests as a PR? That would be lesser work for us to start debugging. Thanks!

timvandenhof commented 3 years ago

@SabotageAndi Sure, I just opend draft PR #2207 for this issue. PR contains test scenario for this issue, not a fix yet. Also updated existing testscenario's to not use the deprecated Register methods.

SabotageAndi commented 3 years ago

Thanks! Having the test should it make it easier to debug to see what is happening

timvandenhof commented 3 years ago

I'm suspecting this part in TEHelper.cs, method GetMembersThatNeedToBeSet:

var properties = (from property in type.GetProperties()
                    from row in table.Rows
                    where TheseTypesMatch(type, property.PropertyType, row)
                        && (IsMemberMatchingToColumnName(property, row.Id())
                        || IsMatchingAlias(property, row.Id()))
                    select new MemberHandler { Type = type, Row = row, MemberName = property.Name, PropertyType = property.PropertyType, Setter = (i, v) => property.SetValue(i, v, null) }).ToList();

TheseTypesMatch will return true because there is a IValueRetriever available, however they are not IsMemberMatchingToColumnName and not IsMatchingAlias. Removing those checks will break other existing tests.

alexandrafung commented 3 years ago

This is not a bug. Using TableAliasesattribute is the solution, not the workaround. In your case, please use "Dutch name" and "English name" aliases.

    public class ParentItem
    {
        public string Name { get; set; }

        [TableAliases("Dutch name")]
        public ChildItem DutchItem { get; set; }

        [TableAliases("English name")]
        public ChildItem EnglishItem { get; set; }
    }
github-actions[bot] commented 3 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.