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

Support for tuples #951

Merged
merged 3 commits into from Oct 24, 2017
Merged

Conversation

UncleFirefox
Copy link
Contributor

@UncleFirefox UncleFirefox commented Oct 7, 2017

This is a work in progress to support tuples on createinstance #950

@SabotageAndi
Copy link
Contributor

Thanks for the PR! Would be a nice feature.

@UncleFirefox you change the line endings in TechTalk.SpecFlow/Assist/TEHelpers.cs. Could you fix that, so that it is easier to see the difference?
I hope that I will find some time on Tuesday to have a look at it.

@UncleFirefox
Copy link
Contributor Author

Hey @SabotageAndi it looks like we'll also need to change the AppVeyor config to have the latest version of the tooling as it seems it's not compatible with tuples.

@SabotageAndi
Copy link
Contributor

If you change the appveyor.yml in a PR, this will be used.
So you can make the needed changes (image: Visual Studio 2017) on your own.

Please also not forget to update the changelog.txt.

@SabotageAndi
Copy link
Contributor

Please also fix the line ending change in TEHelpers.cs.
I can't see the diff here on GitHub.

@UncleFirefox
Copy link
Contributor Author

Hi! I've fixed all line endings. Some things to point out:

  1. I think we got a problem in the whole repo with line endings, we should be consistent or any change will provoke more changes than the ones actually there on PRs
  2. A couple of tests failed that in theory have nothing to do with my PR, will check
  3. I'm more than happy to include how tuples are handled with this new experimental support somewhere in the docs if you tell me where (I guess it's a lot more than just the changelog)

Cheers

@SabotageAndi
Copy link
Contributor

The one failing build was a file system problem. I restarted the build and lets have a look afterwards.

About the line- endings: I think you are right, that they are mixed through the code. We should fix that. @gasparnagy good ideas?

@UncleFirefox: the documentation for the assist helper can be found here: http://specflow.org/documentation/SpecFlow-Assist-Helpers/
Here is the link to edit this page: https://github.com/techtalk/SpecFlow/wiki/SpecFlow-Assist-Helpers/_edit
I think this is the best location to put info about it.

@UncleFirefox
Copy link
Contributor Author

UncleFirefox commented Oct 9, 2017

Thanks! Just updated the wiki to contemplate this new scenario. Looks like build is green now.

I have to say I'm a little worried about the support with the tooling... Is there still support for stuff for VS2010 and old versions of the framework? Just hope this change hasn't broken anything...

@SabotageAndi
Copy link
Contributor

Which tooling do you mean?

@SabotageAndi
Copy link
Contributor

I will have a detailed look at this PR tomorrow.

Copy link
Contributor

@SabotageAndi SabotageAndi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, looks good.
I would like to see some additional tests to be sure the implementation works.

|| genType == typeof(ValueTuple<,,,,,,>)
|| genType == typeof(ValueTuple<,,,,,,,>)
|| genType == typeof(ValueTuple<,,,,,,,>)
|| genType == typeof(ValueTuple<,,,,,,,>)
Copy link
Contributor

Choose a reason for hiding this comment

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

The last 3 are the same. Is this on purpose?

.Any(property => IsMemberMatchingToColumnName(property, firstRowValue));
}

public static bool IsValueTupleType(Type type, bool checkBaseTypes = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some unit tests for this method?
Please add an test, where the tuple has more than 8 properties. Wide tables are not that uncommon and I don't see how this will work with this implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, yes you are right, working with more than 8 elements is quite complicated especially because of how tuples work. I gave it a go but that would imply doing very creepy stuff with memberhandlers and tuple recursion, at any rate people does not recommend to have more than 8 elements in a tuple. What do you think we should do?

  1. Not having support for tuples at all
  2. Have support for tuples with <= 7 elements and throw exceptions for bigger tuples aka DontHaveBigTuplesException
  3. Do creeply and complicated stuff with MemberHandlers

Any preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see 3, but I can live with 2 at the moment.
As I like this feature, I don't want to drop it, only because of this.

@gasparnagy Is 2 for you in the moment also Ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -45,6 +45,9 @@
<Reference Include="System.Core">
<RequiredTargetFramework>3.5</RequiredTargetFramework>
</Reference>
<Reference Include="System.ValueTuple, Version=4.0.1.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51, processorArchitecture=MSIL">
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is a NuGet package and needed by runtime, we need a dependency in our NuGet package for it.
The Nuspec is located here: https://github.com/techtalk/SpecFlow/blob/master/Installer/NuGetPackages/SpecFlow/SpecFlow.nuspec

Please add the dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

}

[Test]
public void Works_with_tuples()
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the CreateInstance/CreateSet method also on this kind of table:

| Name          | Value      |
| PropertyOne   | Look at me |
| PropertyTwo   | hello      |
| PropertyThree | 999        |

Please add a test for this case also.

@SabotageAndi
Copy link
Contributor

@gasparnagy Are you fine with this?
We have now a dependency with System.ValueTuple. It is the oldest available version and it is .NET Standard 1.0/.NET 4.5 compatible. So I don't see a problem with it.

@gasparnagy
Copy link
Contributor

@SabotageAndi @UncleFirefox I like the feature and the implementation also looks ok (I just did a brief look).

The new dependency makes me fear, but hopefully this will not cause too much headache... so ok from my point of view...

Copy link
Contributor

@SabotageAndi SabotageAndi left a comment

Choose a reason for hiding this comment

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

👍

@SabotageAndi SabotageAndi merged commit 88aed25 into SpecFlowOSS:master Oct 24, 2017
@SabotageAndi SabotageAndi changed the title [WIP] Experimental support for tuples Experimental support for tuples Oct 24, 2017
@SabotageAndi SabotageAndi changed the title Experimental support for tuples Support for tuples Oct 24, 2017
@SabotageAndi SabotageAndi mentioned this pull request Oct 24, 2017
18 tasks
Autom8edChaos added a commit to Autom8edChaos/SpecFlow that referenced this pull request Nov 6, 2017
* update MSBuild.Community.Tasks to latest version via NuGet (SpecFlowOSS#801)

* Regexless stepdef fixes (SpecFlowOSS#786)

* Upgrade FluentAssertions

* Add simple unit test

* Add unit test for parametrized method

* Add unit tests for regexless - 36 failing

* fluent assertion does not allow { and } in the because part

* refine expectations: shortening like "doesn't" should not work automatically

* fix regexless matching issues

* a bit more optimal solution that produces simpler regexes

* Improve param locator

* should not remove keywords from binding culture

* small code improvements

* refine expectations: should not allow whitespace or punctuation in the front of the step text (Given !foo) in order to make the generated regex faster (does not start with rule)

* small refactor

* update changelog, remove wip tags

* add test for issue SpecFlowOSS#715

* Update changelog showing that SpecFlowOSS#301 is also fixed

* add CI NuGet package feed

* Added support for MsTest's [DeploymentItem] attribute with @mstest:DeploymentItem tag (SpecFlowOSS#805)

* Added support for MsTest's [DeploymentItem] attribute with @Deploy tag (Issue SpecFlowOSS#803)
* Prefixing new tag with `MsTest:`
* using pascal case @mstest:DeploymentItem
* wrapping nested using blocks with curly brackets

* Change project reader from MSBuild to XML (SpecFlowOSS#837)

* improve tests for project reader
* add tests for new csproj format
* change variable naming
* complete read of classic projects
* refactoring
* mark not used properties as obsolete
* support new project system & refactorings
* use AppDomainIsolatedTask instead of Task to solve file looking issues with MsBuild
* fix typo :-/
* add tests for linked files

* Added display name for theory attribution (SpecFlowOSS#812)

* Added display name for theory attribution
* Fixed incorrect namespace reference to TheoryAttribute (no longer part of extensions)

* Support context parameters on Before/After methods, FeatureContainer (SpecFlowOSS#779)

* Added scenario: Inject FeatureContext into a BeforeFeature hook
* first dummy implementation to make the scenario pass (runtime tests failing)
* Use bindingInstanceResolver to resolve parameters
* make unit tests pass
* Add unit tests for what we have so far
* support for resolving hook parameters from scenario context
* better then step for the scenarios
* fix unit test error
* added failing test for resolving objects from feature container
* refactor ScenarioContext
* Implement FeatureContainer
* move binding culture initialization to FeatureContext
* fix featurecontext resolution from scenario container
* Refactor scenario and feature context and remove displose hack that was necessary to avoid circular disposing loops
* fix unit test failure
* Make InternalContextManager to IObjectContainer reference more explicit
* rename IBindingInstanceResolver to ITestObjectResolver (breaking change for some plugins)

* Wildcard support for project reader (SpecFlowOSS#843)

* Wildcard support for project reader
* fix tests for AppVeyor

* update changelog

* Config file refactoring & Json Config file support (SpecFlowOSS#690)

* split app.config elements to multiple files

* make RuntimeConfiguration nearly immutable

* extract config loading logic to RuntimeConfigurationLoader

* reformating

* add tests for app.config reading

* move ConfigurationTest to separate Namespace

* rename file

* add json tests

* add json.net dependency

* json config file parsing

* add tests for checking which config file is used

* refactoring for tests and added tests

* remove GeneratorConfiguration

* fix dependencies for generator

* fix tests

* fix tests

* move files & fix namespaces

* work on generation config loading

* fix tests and make parsing more robust

* fix and comment test

* small cleanups

* remove ToolLanguage, GeneratorPath and DetectAmbiguousMatches from Config

rename PrintConfigSource to TraceConfigSource

* fix merge issues

* remove file

* add missing json handling

* update changelog

* add Newtonsoft.Json dependency to the NuGet package

* update TestGenerator version

* fix typo in README.md (SpecFlowOSS#853)

* fix VS code behind generation (SpecFlowOSS#855)

* fix redirects
* fix serialization problem with Visual Studio
* fix lineending

* Order sensitive overload for compare to set (SpecFlowOSS#778)

* Add tests
* Refactor TableDifferenceResults to be able to represent order sensitive diff
* make throw tests pass
* refactor message tests to be able to test order sensitive comparison
* fix diff messages for order sensitive
* Replace Tuple with TableDifferenceItem

* Hook with multiple Tags scoped are executed more than once (SpecFlowOSS#848)

* add failing tests
* Fix for SpecFlowOSS#848 Hook with multiple Tags scoped are executed more than once

* making sure that the ordering is preserved (the GroupBy might change the order)

* update changelog for 2.2.0-prerelease20170523

* add GitExtensions configuration file with github and appveyor configuration (SpecFlowOSS#862)

* Added TestThreadContext and exposed it in ContextManager and Steps base class (SpecFlowOSS#875)

* Added TestThreadContext and exposed it in ContextManager and Steps base class

* Upgrade to BoDi v1.3 (SpecFlowOSS#876)

* fix shadow copy test issue
* upgrade BoDi to v1.3
* remove unnecessary BoDi workarounds

* Adds support for xUnit2 ITestOutputHelper SpecFlowOSS#575 (SpecFlowOSS#874)

* Resolves SpecFlowOSS#575 

* added tests for adding support for xUnit2 ITestOutputHelper class

added XUnit2Generator unit tests
added XUnit2Provider specs
updated XUnitExecutionDriver to output results in default (xUnit) xml
output instead of NUnit xml format

* swapped the order of the TestInitialize call and the setting of the _testOutputHelper field

* expose TestAssembly and BindingAssemblies on ITestRunnerManager

* Fix generation from visual studio (SpecFlowOSS#877)

* rename field so that it matches the version of this class from 1.9 (which is used by VS)
* make AppConfig the default value
* add warnings

* Update for 2.2 release

* use FileStream constructor which grants ReadWrite to other processes. (SpecFlowOSS#906)

use FileStream constructor which grants ReadWrite to other processes

SpecFlowOSS#904

* Added changelog entry for PR (SpecFlowOSS#907)

* correct version number

* Access MSTest's TestContext property via ScenarioContext (SpecFlowOSS#882)

* injected MSTest's TestContext into ScenarioContext

* fixed TestContext property generation for VB.Net

* @MSBuild [DeploymentItem] - added option to provide output directory (SpecFlowOSS#901)

* @MSBuild [DeploymentItem] - added option to provide output directory

* code review

* update changelog after merging PRs SpecFlowOSS#882 & SpecFlowOSS#901

* add tests for Feature/Scenario Hooks with context parameters (SpecFlowOSS#925)

* Issue Template (SpecFlowOSS#924)

* Create issue template

* add SpecFlow+Runner to test runners

* Rename issue to ISSUE_TEMPLATE.md

* Allowed custom XSLT scripts to contain scripts.

* Updated changelog for new addition

* enter release date

* fix changelog after 2.2.1 release

* added link to Stack Overflow repro topic (SpecFlowOSS#942)

* Avoid trying to load empty configuration

* Set ConfigSource of default holder to "Default" instead of "AppConfig"

* Turn off line ending git auto conversion (SpecFlowOSS#953)

* Stop git from automatically converting line endings everytime repo is cloned
* add .vs folder to .gitignore

* Fixing copy link in step definition report (SpecFlowOSS#958)

* Update changelog

* Support for tuples (SpecFlowOSS#951)

* Initial support for Tuples
* Adding nuspec dependency
* Added tupple error when there are too many properties

* add entry for tuple

* Make scenario TestResult public (SpecFlowOSS#963)

* Make scenario TestResult public

* - Rename to ScenarioExecutionStatus
- Moved this to the root of the project
- UndefinedStep in enum
- Edited changelog.txt

* Update changelog.txt

* Single agent for unit tests execution

* Fix incorrect appveyour config

* Try to change test_script to test

* Disable automatic tests

* Provide full path to test assemblies

* Expose configuration var to batch

* Hardcoded configuration as Release

* Use %% accessor for configuration var

* Add hyperlink to discussion group (SpecFlowOSS#965)

Remove ambiguity regarding where to go for questions.
@SabotageAndi SabotageAndi added this to the SpecFlow 2.3 milestone Dec 12, 2017
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.

None yet

3 participants