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

Use TestCase with Name instead of Description Attribute for Nunit3 provider. Fix #1225 #1874

Merged
merged 12 commits into from
Feb 25, 2020

Conversation

goblinmaks
Copy link
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added an entry to the changelog

@goblinmaks goblinmaks changed the title Use TestCase with Name instead of Description Attribute for Nunit3 provider. Fix #12 Use TestCase with Name instead of Description Attribute for Nunit3 provider. Fix #1225 Feb 3, 2020
@goblinmaks
Copy link
Contributor Author

#1225

@SabotageAndi
Copy link
Contributor

@goblinmaks For some reason the results of the release pipeline are not shown here.
This PR failed a lot of tests. There is somewhere a null-check that throws an exception.

Release pipeline is here: https://dev.azure.com/specflow/SpecFlow/_apps/hub/ms.vss-releaseManagement-web.hub-explorer?releaseId=1003&releaseEnvironmentId=4495&_a=release-pipeline-progress

@goblinmaks
Copy link
Contributor Author

@SabotageAndi It is not completed commit, I will fix tests, but have additional questions about this PR in #1225 . Could you look at my comment please ?

@goblinmaks
Copy link
Contributor Author

goblinmaks commented Feb 11, 2020

@goblinmaks goblinmaks marked this pull request as ready for review February 11, 2020 12:29
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.

Looks good, except the naming in the unit tests.

Could you post a screenshot of how it looks now in the test explorer?

@goblinmaks
Copy link
Contributor Author

Looks good, except the naming in the unit tests.

Could you post a screenshot of how it looks now in the test explorer?
image

We have a chance to review this PR soon ?

@SabotageAndi
Copy link
Contributor

Sorry for not getting earlier back to you. I have a lot of work currently to do.

I was not clear enough what I meant with the screenshot. I wanted to see how this change affects the display of the scenarios in the test explorer. Not how the unit tests you added look like.

@goblinmaks
Copy link
Contributor Author

@SabotageAndi nothing changes after this PR will be applied.
image

image

@SabotageAndi
Copy link
Contributor

@goblinmaks Sure things changed. The feature and scenario names are now nice. Before there weren't spaces. Now they are here.
I thought something like this will probably happen. And it changed to be better.

Thanks for your contribution to SpecFlow. If you would like us to send you some SpecFlow stickers as a thank you, please fill out the form here.

@SabotageAndi SabotageAndi merged commit bf8a8e1 into SpecFlowOSS:master Feb 25, 2020
@LiohAu
Copy link

LiohAu commented Mar 7, 2020

@goblinmaks Is it normal that the TEST_ATTR constant ("NUnit.Framework.TestAttribute") is not used anymore after your PR ?

Just saw that because I am looking for a way to add the attributes below on the generated class.

[TestFixture(Platform.iOS)]
[TestFixture(Platform.Android)]

Until now, I was using a partial class + an abstract base class to complete the one generated by SpecFlow with those attributes, but it seems nunit does not detect inherited test fixtures anymore :(

@goblinmaks
Copy link
Contributor Author

@LiohAu

Is it normal that the TEST_ATTR constant ("NUnit.Framework.TestAttribute") is not used anymore after your PR ?

I think it is normal, instead of NUnit.Framework.TestAttribute we start use NUnit.Framework.TestCaseAttribute that is allowed to provide TestName property to engine.

Could you share your code, to provide info how you use additional parameter like "platform" in [TextFixture] attribute. Currently we still generate [TextFixture] code attribute but with parameter TestName [TestFixture(TestName="BlaBla")]

How you use code with my commit ? It is not released now due to SpecFlow CI build error.

@nvborisenko
Copy link
Contributor

I wouldn't recommend to use TestCaseAttribute for regular scenarios. This attribute is designed for data driven tests, when the same test can be executed many times on different sets of data. Using it may cause other issues in nunit reporting, like nunit engine creates new hierarchical level of tests. So, regular scenario in specflow will be considered as a suite w/ single test in terms of nunit.

@goblinmaks
Copy link
Contributor Author

@nvborisenko
Currently we have no other ability to set Test Name for for NUnit test.
For my point of view TestCaseAttribute should be used for Scenario Outline as minimum.
We can try add TestName to TestAttribute on nunit side, in case if this will be applied we can use it for regular Scenario.

It just my opinion.

@nvborisenko
Copy link
Contributor

Right, this way is better. We will not see collisions in tests tree.

Now you see "good" tests tree in VS Test Explorer because VS applies some magic in building it. VS hides tree nodes. I guess if you execute tests with any runner, you will see unexpected "namespace" for regular scenarios (which are not outlined).

@goblinmaks
Copy link
Contributor Author

nunit/nunit#3285

SabotageAndi added a commit to SabotageAndi/SpecFlow that referenced this pull request Mar 31, 2020
SabotageAndi added a commit that referenced this pull request Apr 1, 2020
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.

4 participants