Skip to content

Conversation

@clrudolphi
Copy link
Contributor

@clrudolphi clrudolphi commented Jun 5, 2025

🤔 What's changed?

PR to make the new project wizard data-driven and pull dependency information from an http json file.

⚡️ What's your motivation?

The New Project Wizard should create Reqnroll projects using the latest release of Reqnroll (without also requiring a re-release of the VS Extension).

🏷️ What kind of change is this?

  • ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

As always, thread management is an issue; The http request is being made via Task.Run to keep it off of the UI thread. Please review.

📋 Checklist:

  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

@clrudolphi clrudolphi requested a review from gasparnagy June 5, 2025 22:53
"name": "Reqnroll.NUnit",
"version": "2.4.1"
},
"TestFramework_Lib": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I fear that there will be test frameworks that require more than one dependencies (maybe in some combinations we might need even multiple Reqnroll dependencies too), so I think I would prefer having a general list of dependencies that we just add and not have that categorization.

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 understand your rationale. When allowing for a more flexible set of package references, the wizard code must then generate a <PackageReference> string for each and the number of them can't be predicted ahead of time. That means the template would have a single substitution token into which all of the package reference strings must go.

I've spent all afternoon on this and keep running in to the same problem.
Template substitution works fine when the substitution variable represents a single xml element to be injected into the csproj (or a subsection of an element's textual representation).

When there are multiple items to be injected, each has to have its own substitution variable. Any attempt to include more than one line or more than one xml element causes an error.

I've attempted every way I can think of to join multiple together, but MSBuild keeps complaining that there is invalid xml (specifically 'The element <#text> beneath element <ItemGroup> is unrecognized'). I've joined the package reference strings with Environment.NewLine, with a hard-coded '\n' and with no separation at all. All result in the same error.

Have you seen this problem before? (I'm struggling to find reliable docs and information on what is/is-not allowed)

Copy link

@Code-Grump Code-Grump Jun 7, 2025

Choose a reason for hiding this comment

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

It sounds like the system is rejecting XML fragments. Could you add a complete <ItemGroup> with all its descendants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is rejecting multi-line substitution variable values.
I tried the suggestion of substituting the entire <ItemGroup> element, but ended up with the same error.

@clrudolphi
Copy link
Contributor Author

Here is a working example. I went with an approach that supports up to 10 PackageReferences supported. Each reference gets its own line in the csproj template and a flag indicating whether it has a value.
Admittedly, it is a bit of a hack and I can go back and try a few other things, but the other approaches I have in mind bypass the templating and directly manipulate the Project model in the DTE.

@gasparnagy
Copy link
Contributor

Here is a working example. I went with an approach that supports up to 10 PackageReferences supported. Each reference gets its own line in the csproj template and a flag indicating whether it has a value. Admittedly, it is a bit of a hack and I can go back and try a few other things, but the other approaches I have in mind bypass the templating and directly manipulate the Project model in the DTE.

I think this might have been some other issue. I have tried with single param and it seemed to work. Please check my commit. It's just a dummy implementation, but seems to work.

@clrudolphi
Copy link
Contributor Author

I think this might have been some other issue. I have tried with single param and it seemed to work. Please check my commit. It's just a dummy implementation, but seems to work.

Ahgh. OK, thanks for getting me started down the right path. I suspect that doing only "Build" instead of "Rebuild" when modifying the template does not properly package the changed template into the extension. I was repeatedly testing something that I thought I was changing but really was not. Lesson learned.

@clrudolphi
Copy link
Contributor Author

So, let's talk about revisions to the content of the dialog box. We have the 'description' field that we could use. We could add a list of possible validation frameworks to use in a dropdown, etc.
Here is the as-is:
NewProjAsIs

Here is a mock-up of what it could look like:
NewProjToBe

Do you want to add these new features to the dialog?
Where would you like to see the placement of the Test Framework description text? (Appended to the name of the test framework as shown in the mock-up, or perhaps as a line of text either below or to the right of the dropdown that would get updated when a new selection is made)

Should each dropdown entry include the version number, and we provide for multiple versions in the dropdown of a given item? (In case an enterprise has standardized on a version they can select that instead of getting always the latest. I mean, within reason, no need to keep more than just a handful of each item.)

@clrudolphi
Copy link
Contributor Author

Another question is when building the set of metadata for the view model and we retrieve data from the Reqnroll.net site. We need a way to allow for that response to be mocked for unit testing, and we need a way for that response to be mocked when testing locally (but still test proper operation of the HTTP request). Do we need a test page url that provides test content?

What did you have in mind in terms of the authoring/approval/deployment process for that json file?

@gasparnagy
Copy link
Contributor

Where would you like to see the placement of the Test Framework description text? (Appended to the name of the test framework as shown in the mock-up, or perhaps as a line of text either below or to the right of the dropdown that would get updated when a new selection is made)

I would place the additional information below the dropdown. In addition to the description we could get a URL to the particular framework as well (e.g. https://xunit.net/) that we could display as:

<dropdown>
<description>
More information: <link>

(Maybe we don't even need the description. I don't know. What do you think?)

I would not bother with the validation and mock framework (at least not in this PR). That would need some further consideration, especially if they are commercial.

Should each dropdown entry include the version number, and we provide for multiple versions in the dropdown of a given item?

No for both. I don't think so.

Another question is when building the set of metadata for the view model and we retrieve data from the Reqnroll.net site. We need a way to allow for that response to be mocked for unit testing, and we need a way for that response to be mocked when testing locally (but still test proper operation of the HTTP request). Do we need a test page url that provides test content?

I usually solve that kind of problem in a way, that in DEBUG mode, I use a different file name. So for example if the final would be https://something.reqnroll.net/test-frameworks.json then for debug it can be https://something.reqnroll.net/test-frameworks.dev.json. This way you can do manual exploratory testing without modifying the final description file.

What did you have in mind in terms of the authoring/approval/deployment process for that json file?

We have a site called https://schemas.reqnroll.net where we host the config schema file. One easy option would be to store this description file also there. The schemas.reqnroll.net is currently a folder at my hosting provider (ssh/scp/sftp), but we could turn it into a GitHub Pages site, so a modification of the json file would be a PR+merge (or direct push for the "dev" file) to that repo. What do you think?

@clrudolphi
Copy link
Contributor Author

clrudolphi commented Jun 11, 2025

What do you think?

I prefer the GitHub approach. The process is already well understood by the team and you can avoid sharing of secrets.

I think it would be straightforward to host this using the Reqnroll Jekyll site. From what I understand we just need to define a .json page with a null template.

I would like to modify the json structure to make it easier to later add the validation and other frameworks by putting the array of test frameworks inside a property identifier (such as "reqnroll").

@gasparnagy
Copy link
Contributor

I prefer the GitHub approach. The process is already well understood by the team and you can avoid sharing of secrets.

OK.

I think it would be straightforward to host this using the Reqnroll Jekyll site. From what I understand we just need to define a .json page with a null template.

I don't want to mix this with the reqnroll website (reqnroll.net domain), because if we ever need a more dynamic site again, then better to keep the static assets separate. This is why the schemas.reqnroll.net domain is a good candidate. But we can make a new sub-domain as well, like assets.reqnroll.net as well, what @Code-Grump suggested.

For hosing such an asset site with GitHub pages, we don't even need Jekyll. Just a "static" web site. I'll prepare the GitHub repo for that.

I would like to modify the json structure to make it easier to later add the validation and other frameworks by putting the array of test frameworks inside a property identifier (such as "reqnroll").

Could you please paste an example of that here?

@clrudolphi
Copy link
Contributor Author

Could you please paste an example of that here?
I was thinking something along the lines of:

{
   "testFrameworks": [
	  {
		"label": "xUnit",
		"description": "Use xUnit v2 test executor with Reqnroll",
		"url" : "https://xunit.net",
		"dependencies": [
		  {
			"name": "Reqnroll.xUnit",
			"version": "2.4.1"
		  },
		  {
			"name": "xunit",
			"version": "2.8.1"
		  },
		  {
			"name": "xunit.runner.visualstudio",
			"version": "2.8.1"
		  }
		]
	  },
	  {
		"label": "MsTest",
		"description": "Use MsTest v3 test executor with Reqnroll",
		"url" : "https://github.com/microsoft/testfx?tab=readme-ov-file",
		"dependencies": [
		  {
			"name": "Reqnroll.MsTest",
			"version": "2.4.1"
		  },
		  {
			"name": "MSTest.TestFramework",
			"version": "3.4.3"
		  },
		  {
			"name": "MSTest.TestAdapter",
			"version": "3.4.3"
		  }
		]
	  },
	  {
		"label": "NUnit",
		"description": "Use NUnit v3 test executor with Reqnroll",
		"url" : "https://nunit.org",
		"dependencies": [
		  {
			"name": "Reqnroll.NUnit",
			"version": "2.4.1"
		  },
		  {
			"name": "nunit",
			"version": "3.14.0"
		  },
		  {
			"name": "NUnit3TestAdapter",
			"version": "4.5.0"
		  }
		]
	  }
    ],
	"frameworks": [
		{
			"tag": "net462",
			"label": ".NET Framework 4.6.2"
		},
				{
			"tag": "net47",
			"label": ".NET Framework 4.7"
		},
		{
			"tag": "net471",
			"label": ".NET Framework 4.7.1"
		},
		{
			"tag": "net472",
			"label": ".NET Framework 4.7.2"
		},
		{
			"tag": "net48",
			"label": ".NET Framework 4.8"
		},
		{
			"tag": "net48",
			"label": ".NET Framework 4.8.1"
		},
		{
			"tag": "net6.0",
			"label": ".NET 6.0"
		},
		{
			"tag": "net7.0",
			"label": ".NET 7.0"
		},
		{
			"tag": "net8.0",
			"label": ".NET 8.0",
			"default": true
		},
		{
			"tag": "net9.0",
			"label": ".NET 9.0"
		}
	],
	"validationFrameworks": [
	  {
		"label": "FluentAssertions",
		"description": "Use Fluent Assertions library with Reqnroll",
		"url" : "https://fluentassertions.com",
		"dependencies": [
		  {
			"name": "FluentAssertions",
			"version": "8.3.0"
		  }
		]
	  }
	]
}

@gasparnagy
Copy link
Contributor

I was thinking something along the lines of

I like it

@clrudolphi
Copy link
Contributor Author

I usually solve that kind of problem in a way, that in DEBUG mode, I use a different file name. So for example if the final would be https://something.reqnroll.net/test-frameworks.json then for debug it can be https://something.reqnroll.net/test-frameworks.dev.json. This way you can do manual exploratory testing without modifying the final description file.

I'm thinking of several different testing scenarios and each might need a slightly different approach. Which are worthy of pursuing?

  • Developer builds locally in DEBUG and hits reqnroll.net to retrieve a test-data manifest json. (This requires the ability to change the document name retrieved from reqnroll.net)
  • Developer builds locally and retrieves the test-data manifest from a local http endpoint on his/her local network. This requires the ability to change the entire URL at test time.
  • Developer builds locally and wants to test that updates (content or structure) to fall-back json file is correct; doing so requires that the network fetch of the json file be skipped.

This would lead to needing three settings that could be overridden when running the experimental instance of VS. Would you prefer to see these shared via Environment variables or command line params (either of which can be set in the Debug Options for launching the experimental instance). The three settings would be: name of file to retrieve, base url, boolean whether to skip network fetch.

I would also think we'd want the MetaDataProvider (the class that is reading this data from the web) would be written in a testable way, allowing for mocks of the http request/result, etc). Presuming we have those tests in place, how much of the above exploratory testing capability is really required?

@gasparnagy
Copy link
Contributor

This would lead to needing three settings that could be overridden when running the experimental instance of VS. Would you prefer to see these shared via Environment variables or command line params (either of which can be set in the Debug Options for launching the experimental instance). The three settings would be: name of file to retrieve, base url, boolean whether to skip network fetch.

I would also think we'd want the MetaDataProvider (the class that is reading this data from the web) would be written in a testable way, allowing for mocks of the http request/result, etc). Presuming we have those tests in place, how much of the above exploratory testing capability is really required?

The exploratory tests are not so much for the code itself but for the metadata updates. I mean I add a new item to the metadata and I would like to quickly check if that looks ok in the app itself before publishing it to all users.

But for that maybe the best would be is to provide a single env variable that can be set to override the URL and then we don't need the DEBUG magic, the file renames and any other complication. And you can set it to a local endpoint or even to an invalid or 404 url to test the system. Maybe that would be the easiest. This would be an internal config, I would not document this as a public configuration option.

EnvironmentVariable is available to override the URL for testing purposes.
Added Description and HyperLink content to the dialog box.
@clrudolphi
Copy link
Contributor Author

Added implementation as discussed. I'm not a xaml expert, only dabbled in it years ago. I don't have the xaml designer workload installed on my VS installation either. So, it's likely that I've mucked things up a bit. Please review and change what you don't like.
There is one problem that I can't seem to solve - the framework description TextBlock is displaying black text on a black background on my dark-themed VS installation. I could use some help with that.
I'll continue work by adding unit tests for the classes I've added.

…ion for Env Variable access to facilitate testing.
@clrudolphi
Copy link
Contributor Author

Added unit tests.
Getting some strange test failures in the tests of the connectors (I haven't changed any of that code, so I don't know whats going on, would you check please?)

@clrudolphi clrudolphi requested a review from gasparnagy June 13, 2025 17:09
@gasparnagy
Copy link
Contributor

I have "fixed" the styling by replacing the TextBlock with label. I don't remember fully how the theme support works (I remember it was quite a hack), and I could not figure out how to apply it for the TextBlock.

The data binding is still has some issues, but I don't have more time right now. I will continue on that tomorrow.

@gasparnagy
Copy link
Contributor

Getting some strange test failures in the tests of the connectors (I haven't changed any of that code, so I don't know whats going on, would you check please?)

For me they pass. What error do you get?

@clrudolphi
Copy link
Contributor Author

Getting some strange test failures in the tests of the connectors (I haven't changed any of that code, so I don't know whats going on, would you check please?)

For me they pass. What error do you get?

The Reqnroll.VisualStudio.ReqnrollConnector.Tests. Generated ProjectTests - Approval fails.
All of the Specs fail with a complaint that "0006 0:00.148179 Info 19 AndBindingSourceIsValid:Test assembly not found. Please build the project to enable the Reqnroll Visual Studio Extension features."

@gasparnagy
Copy link
Contributor

The Reqnroll.VisualStudio.ReqnrollConnector.Tests. Generated ProjectTests - Approval fails.
All of the Specs fail with a complaint that "0006 0:00.148179 Info 19 AndBindingSourceIsValid:Test assembly not found. Please build the project to enable the Reqnroll Visual Studio Extension features."

That error can mean too many things... ☹️ Any other detail in the log? Or exception if you try to debug it?

@gasparnagy
Copy link
Contributor

I have reviewed the view model data binding and the metadata fetching.

The current way of metadata fetching was not good, because actually the retrieval went synchronously. You provided a callback delegate, but the invocation of the delegate was sync. The code could have been refactored in a way that the invocation of the delegate is from the background thread, but in general, for UI it is much better to use async, because updating UI from a background thread is not working and you would need to do additional magic to support that.

So I have refactored the NewProjectMetaDataProvider class to provide an async task that eventually returns the loaded data. The view model has been also refactored, because from the ctor you cannot invoke that async method, so it has now an InitializeAsync method that is invoked by the dialog as an async event. The refactoring is not fully complete, I haven't fixed the tests yet, hence the old method is still there. Also some fields in NewProjectMetaData are not needed now. Could you please finish that?

As the metadata loading might take some time, I also changed the "fallback" mechanism in a way that the view model is populated with the fallback data in the ctor, that is overridden by the value that comes from the InitializeAsync. This way the user already sees something even before the real data comes. I'm not sure this is perfect, because if they change the selection while the metadata is loading, the selection will be reset after the arrival... Probably the best would be to start the metadata prefetch even earlier (when our package loads), so that once the dialog opens the metadata is already in place. But let's leave this optimization for later, once we see how quickly the data comes.

I have also noticed, that for the unit test providers, we don't have a separate "tag" and "label". I think it would make sense to have that (like for .NET frameworks). The "tag" would be the key that we pass for telemetry and the "label" would be the one we display in the dropdown. I did half of this already: the view model now supports a separate label, but from the data both are fed by the label.

I haven't done a complete review yet, were only focusing on this part.

Development suggestions:

  • We have a Reqnroll.VisualStudio.UI.Tester project that is a windows desktop app for testing the UI components, like dialogs without the need of starting VS every time. It has a main screen with buttons to test the different parts. There was already a button for testing the AddNewReqnrollProjectDialog with test view model, but now I added one that allows you to test it with the real NewProjectMetaDataProvider. With that it is much quicker to develop such things.
  • Also you can test the async behavior of the data by adding a Task.Delay(1000) line into the NewProjectMetaDataProvider.RetrieveNewProjectMetaDataAsync method. (Just don't forget to remove. 😉)

… the tests to accommodate the new structure.

Added tag concept to models used to describe TestFrameworks.
@clrudolphi
Copy link
Contributor Author

I modified the tests to adapt to the new execution model.
I also extended the concept of tag for the TestFrameworks thoughout.

Take a look when you have time.

@clrudolphi clrudolphi marked this pull request as ready for review August 12, 2025 15:42
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