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

Move generated files outside of project source #1983

Open
wants to merge 283 commits into
base: master
Choose a base branch
from

Conversation

Code-Grump
Copy link
Contributor

@Code-Grump Code-Grump commented May 10, 2020

Removes the generated codebehind files from the project structure. Generated files are now written to a subdirectory in the /obj/ folder.

  • Change MSBuild targets for build and clean to generate files as a build artefact, prior to compilation.

There will need to be some advice given on upgrading, i.e. all feature.cs files should be removed from the project folders, and the documentation at https://specflow.org/documentation/Generate-Tests-from-MsBuild/ could possibly benefit from some information about the few properties available to customize the process.

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).
  • Performance improvement
  • Refactoring (so no functional change)
  • Other (docs, build config, etc)

Checklist:

  • I've added tests for my code. (most of the time mandatory)
  • I have added an entry to the changelog. (mandatory)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@Code-Grump
Copy link
Contributor Author

This PR replaces #1691

@Code-Grump
Copy link
Contributor Author

It looks like the build failure might be transient. Is there any way to run the failed test again?

@SabotageAndi
Copy link
Contributor

Since another fix, we get this kind of error sometimes in our CI pipeline. No idea yet why. You can ignore it.

I will try to find some time on Monday/Tuesday to review the PR.

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.

Only 2 small changes to made.
I like that this make the MSBuild stuff easier.

Before I merge this, I want to try it out and see how Visual Studio behaves when the files are not in the same folder.

changelog.txt Outdated Show resolved Hide resolved
@SabotageAndi
Copy link
Contributor

I tried out the latest version of this PR.
Sadly as I assumed, moving the files into the obj folder breaks the navigation from the Test Explorer to the feature file. This is working for SDK- style projects, but after this change, this is not working anymore.

I have here a solution where I tested it. It has projects for every unit test framework in old and SDK- style project format.

As far as I know and remember the test explorer has some code for SpecFlow to make this navigation work. What I don't know if this depends on the DependentUpon property of the Compile item or simple that the file is in the same folder.

When I tested it for the old project format, I also saw, that the code-behind file is not generated in the obj folder.
image

@Code-Grump
Copy link
Contributor Author

Thanks for checking it out, @SabotageAndi.

The failure to generate feature-files for classic project formats is pretty unexpected. I'm going to guess that the property I've tried to use wasn't defined in the MSBuild version you used; it should be an easy fix using different MSBuild props. I will focus on getting this working first.

When I tested locally, the source-link was working for SDK-style projects; I did not test for classic projects. I will have to try repeating my tests locally. I thought this was driven primarily by having a #line directive in the generated feature files. I suspect this may be the harder of the two issues to resolve.

@SabotageAndi
Copy link
Contributor

Interesting. Which VS Version are you using?

@Code-Grump
Copy link
Contributor Author

I'm using Visual Studio Professional 2019 (16.6.0)

@Code-Grump
Copy link
Contributor Author

I've cloned and built the master branch of the test project as a baseline. In the cases where source link is working, I can see #line directives being emitted in the code-behind files.

Looking at the files being generated by my branch, there are no #line directives. Having looked at some other projects using the current release of SpecFlow, I can see they also don't have these #line directives and display "No source available" when examined in the test explorer.

Is there something specifically required to have these directives emitted? If I can see it in production use, I'm wondering if I've unexpectedly tripped this behaviour.

@oysteinkrog
Copy link

Is there anything external contributors can do to assist with this issue?
It's quite annoying for most of the developers in our team and it would be nice to fix it.

@Code-Grump
Copy link
Contributor Author

The outstanding issue is having the Visual Studio Test Explorer be able to link the test to its source source (the .scenario file). If anybody could produce a source file in the /obj path which worked to link back to a scenario file, the generator could be updated to produce it and the ticket could progress.

Comment on lines 92 to 84
<Target Name="CleanGeneratedSpecFlowCodeBehindFiles">
<Delete Files="$(SpecFlowCodeBehindOutputPath)**/*$(DefaultLanguageSourceExtension)" ContinueOnError="true" />
</Target>
Copy link

Choose a reason for hiding this comment

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

Rather than manually deleting the files, consider adding them to FileWrites item (in addition to Compile). MSBuild will take care of deleting them when needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not familiar with this item group. An initial read of the documents suggests some possibly timing issues about when these files are generated, but otherwise it looks like a great enhancement to consider.

Copy link

Choose a reason for hiding this comment

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

I second this suggestion. It might be worth taking a look over how AssemblyInfo.cs is generated, as it's a similar pattern wanted here:
https://github.com/dotnet/sdk/blob/db74337457686f042cebdcb613b47fb5d2f14342/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAssemblyInfo.targets

In time we could adopt a similar caching mechanism, to avoid rewriting files at build when none of the inputs have changed.

You'll see the file path is $(IntermediateOutputPath)$(MSBuildProjectName).AssemblyInfo$(DefaultLanguageSourceExtension), and it adds the generated .cs file to Compile & FileWrites items.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incremental support would be quite easy to add by declaring inputs and outputs on the GenerateSpecFlowCodeBehindFiles target. It's something I had intended to add once the generation process worked reliably, but introduces additional complexity in the meantime.

@@ -14,6 +14,7 @@


<PropertyGroup>
<SpecFlowCodeBehindOutputPath Condition="'$(SpecFlowCodeBehindOutputPath)' == ''">$(BaseIntermediateOutputPath)Features.Generated\</SpecFlowCodeBehindOutputPath>
Copy link

Choose a reason for hiding this comment

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

I think it is better to use IntermediateOutputPath to avoid any conflicts between different configurations. It points to configuration specific folder inside BaseIntermediateOutputPath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe there was an issue with doing so, and I agree it would be preferable. My primary focus is getting this process to work with the Test Explorer, but will be happy to examine this aspect again once that is resolved.

Copy link

Choose a reason for hiding this comment

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

The issue is props file is imported early and IntermediateOutputPath will not be set. It will work if you move this to targets file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would create a problem trying to include the generated files in the CustomAddtionalCompileInputs item group, which is part of the fast-update check.

Copy link

Choose a reason for hiding this comment

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

What is CustomAdditonalCompileInputs for? I don't see it being consumed anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a CustomAdditionalCompileInputs item group defined in Microsoft.CSharp.Core.targets and the CoreCompile target depends on this. It is used to enable incremental builds as part of the FastUpdateCheck target, which is used only by Visual Studio and not command-line builds.

If these targets and groups are not catered for, situations arise where builds are considered "up-to-date" by Visual Studio and the build process is skipped entirely until a rebuild is forced.

Copy link

Choose a reason for hiding this comment

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

Interesting, I never came across it. But if CoreCompile consumes it then that can also be moved to targets file.

Copy link

Choose a reason for hiding this comment

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

Did you try moving this and CustomAdditionalCompileInputs etc. to targets file?

@nvmkpk
Copy link

nvmkpk commented Oct 20, 2020

The issue here I think is because the files are generated at build time and Test Explorer does not read the pdb. This can be confirmed by opening the generated files in visual studio, you will notice that the files are not part of any project. The code generation needs to happen at design time for test explorer to recognize them. This also means that the files will be generated whenever the feature file is saved.

@Code-Grump
Copy link
Contributor Author

The issue here I think is because the files are generated at build time and Test Explorer does not read the pdb.

The current code-generation also runs at build-time, but generates the files within the scope of the source files. What makes you believe the PDB is the issue? Could you demonstrate this?

@nvmkpk
Copy link

nvmkpk commented Oct 21, 2020

I am not saying PDB is the issue. When files are generated at build time, Visual Studio does not know they are part of project. This can be proved by opening the generated files in Visual Studio and the project dropdown on the top left of the editor window showing 'Miscellaneous Files'. Since the generated files are not part of project, Test Explorer is also not able to find the source of the test.

I tried this in a different way and it partially works. Currently the code generation targets run before the Build target. If I change that to run them before BeforeCompile target, Visual Studio (and Test Explorer) correctly recognizes the generated files as being part of project. However, navigation to the source still fails and it looks to be an issue with Test Explorer itself. I can show you what my changes look like if you are interested.

@Code-Grump
Copy link
Contributor Author

At present, everything works correctly with this feature branch except the navigation to source in Test Explorer. The expectation is that the explorer should link to the feature file, not the generated files, but this is proving to be nearly impossible to diagnose without having a deeper understanding of the test explorer itself.

@slang25
Copy link

slang25 commented Oct 21, 2020

A shameless nerd-snipe, but I wonder if @nohwnd can help us with understanding how the navigation with Test Explorer works or can point us in the direction of someone who can.

@nohwnd
Copy link

nohwnd commented Oct 21, 2020

Is there a simple repro we can use to investigate this?

@shyamnamboodiripad Is there a way to redirect the test object to a different source than the one that is used to run the test? I doubt there is. If there isn't should this be made part of the FQN effort?

@Code-Grump
Copy link
Contributor Author

There's no simple repro for this at the moment, but I could create a repo which simulates the generator:

  • Having a feature file in the source tree
  • Including an "external" source file in the compile process which uses #line directives to map to the feature file

The current SpecFlow generation process (which includes the generated files within the source tree) does link correctly via the Test Explorer. We'd like to be able to hook into that same process.

@nvmkpk
Copy link

nvmkpk commented Oct 21, 2020

At present, everything works correctly with this feature branch except the navigation to source in Test Explorer. The expectation is that the explorer should link to the feature file, not the generated files, but this is proving to be nearly impossible to diagnose without having a deeper understanding of the test explorer itself.

When I tried with the changes in this PR, Test Explorer did not even show the feature file name that the test was generated from. I got it to show the feature file name and line number.

@nvmkpk
Copy link

nvmkpk commented Oct 21, 2020

I got this working. Initially I tried my changes in master branch. When I applied my changes on top of this PR, navigation from Test Explorer to feature file worked exactly as expected.

@Code-Grump
Copy link
Contributor Author

I got this working. Initially I tried my changes in master branch. When I applied my changes on top of this PR, navigation from Test Explorer to feature file worked exactly as expected.

Can you identify which changes specifically made a difference or provide a working branch?

SabotageAndi and others added 19 commits March 29, 2023 14:58
…SpecFlowOSS#2667)

* BindingInvoker changed to pass AggregateExceptions through. Use of 'PreserveStackTrace' replaced with call to ExceptionDispatchInfo.Capture.
BindingInvoker unit tests added to validate handling of various combinations of exceptions thrown from sync and async StepDefinition methods.

* BindingInvoker now passes AggregateExceptions along up the call chain. BindingInvoker changed to pass AggregateExceptions through. Use of 'PreserveStackTrace' replaced with call to ExceptionDispatchInfo.Capture.
BindingInvoker unit tests added to validate handling of various combinations of exceptions thrown from sync and async StepDefinition methods.
TestExecutionEngine updated to also use ExceptionDispatchInfo.Capture().Throw() in order to preserve the stack trace of user-hook's exception.
…uring test execution (SpecFlowOSS#2663)

* Refactor BindingSourceProcessor to collect binding errors

* Fix BindingSourceProcessorStub

* Add tests & fixes

* collect type load errors

* Extend changelog

* Replace tuple with a class on the IBindingRegistry interface
…subsequent binding method invocations (SpecFlowOSS#2658)

* first idea to fix SpecFlowOSS#2600

* Improve idea to work on .NET 4

* Make BindingInvoker work with Task<T>

* fix execution for failing binding methods

* Keep ExecutionContext in ScenarioContext

* Cleanup unit tests
The sentence "Shouldn't have a return type or have `Task` as return type." can be read in two ways:
1. Should not have a return type nor return `Task`
2. Should not have a return type OR should return `Task`

So a minor change to make it more clear and direct.
* Add support for Rule Backgrounds. Preliminary commit. Lacks tests.

* Add support for Rule Backgrounds.Revised Commit to simplify approach and code. Lacks tests.

* Refactored signature of method: UnitTestMethodGenerator.GenerateTestMethodBody to accept a ScenarioDefinitionInFeatureFile instead of an AST Scenario. This allows simpler tracking of the relationship between Scenario and Rule.
Refactored Rule Background support in ScenarioPartHelper to simplify the code and accept changes suggested during PR review.

* Adding a feature file in the Specs tests to demonstrate a Feature with multiple Rules, each having  a Background; the Background steps should only be executed for the Scenario(s) within their respective Rule.

* Further simplification of Rule support in SPH by merging two private methods together.

* Corrected feature's binding code. Tests now pass.

* Updating changelog

* Reverting this feature back to the version present in SpecFlowOSS/SpecFlow.

* Refactored/revised to improve readabliity and formatting

Co-authored-by: Gáspár Nagy <gaspar.nagy@gmail.com>
@Code-Grump
Copy link
Contributor Author

After 10,000 years, I have returned to make this work.

I have merged everything from master to make this PR up-to-date and without conflicts and now all the automated checks have passed successfully. 👍

@Code-Grump
Copy link
Contributor Author

@SabotageAndi As it's been a very long time in-progress, is there anything new or different that you'd like me to do in order to progress this PR?

@SabotageAndi
Copy link
Contributor

@Tragedian yeah, very long time. And now a bad timing.
I left Tricentis and SpecFlow at the end of February and don't work on it anymore.

@Code-Grump
Copy link
Contributor Author

Code-Grump commented Apr 4, 2023

@Tragedian yeah, very long time. And now a bad timing. I left Tricentis and SpecFlow at the end of February and don't work on it anymore.

In that case, may I wish you the best of fortune in wherever you should go next.

Is there anybody left maintaining this project in your absence?

@SabotageAndi
Copy link
Contributor

Tbh, I don't know. I started the process of handover, but there wasn't much progress until I left.

@ngbrown
Copy link
Contributor

ngbrown commented Apr 20, 2023

What about implementing ISourceGenerator and creating a .NET Compiler Platform ("Roslyn") SDK Source Generator?

It seems all of the MSBuild manipulation could go away?

@Code-Grump
Copy link
Contributor Author

What about implementing ISourceGenerator and creating a .NET Compiler Platform ("Roslyn") SDK Source Generator?

I think this would be an excellent longer-term goal for the SpecFlow project, but it would be a much bigger piece of work to implement a new generator pattern than changing where the current generator outputs its generated sources.

@gasparnagy
Copy link
Contributor

@Code-Grump I have just scanned through this conversation, but it is pretty long. Could you please give a brief status of this PR? I am now interested on these:

  • Is the PR "ready" from your point of view? Any outstanding questions?
  • Was the VS Test Explorer navigation issue clarified/fixed? Is it working now for VS2019 & VS2022 for a "usual" project?
  • Did you plan to have some backup switch that would restore the original behavior just in case someone depends on that or if there is an unexpected circumstance that the new way does not handle?

@Code-Grump
Copy link
Contributor Author

Code-Grump commented Oct 25, 2023

Is the PR "ready" from your point of view? Any outstanding questions?

I have no outstanding items that I'm aware of. Really just needs some input from real user experiences.

Was the VS Test Explorer navigation issue clarified/fixed? Is it working now for VS2019 & VS2022 for a "usual" project?

The test-explorer navigation now works the same as my current experience; the test explorer takes you to the feature file related to the test-run.

Did you plan to have some backup switch that would restore the original behavior just in case someone depends on that or if there is an unexpected circumstance that the new way does not handle?

There is no way to switch to the old behaviour with this current implementation. It could be added via an MSBuild property, but the changes are significant enough that I would probably want to create a new branch and re-implement my changes.

@gasparnagy
Copy link
Contributor

@Code-Grump Thank you for the summary.

I agree that getting feedback from broader user base would be useful as this is a bigger change.

So my suggestion would be not to include it to v4.0 as it would delay the release too much, but immediately after v4.0 is out we should merge this and release as v4.1 beta, to get more feedback.

Don't worry about the opt-out setting yet, we can decide on it based on the v4.1 beta feedback.

Thanks for the contribution, this is a very good feature. I also don't like having the generated files in the feature's folder.

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.