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.

Sign up for GitHub

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

Jump to bottom

xunitv3 #7924

Merged
merged 14 commits into from
Mar 13, 2025
Merged

xunitv3 #7924

merged 14 commits into from
Mar 13, 2025

Conversation

danmoseley
Copy link
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #7225

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@Copilot Copilot bot review requested due to automatic review settings March 6, 2025 04:17
Copilot
Copilot AI reviewed Mar 6, 2025
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 36 out of 36 changed files in this pull request and generated no comments.
@danmoseley danmoseley marked this pull request as draft March 6, 2025 04:17
@danmoseley danmoseley requested a review from DamianEdwards March 6, 2025 04:17
danmoseley and others added 7 commits March 7, 2025 12:05
@danmoseley @DamianEdwards
Update src/Aspire.ProjectTemplates/templates/aspire-xunit/.template.c…
5f9725a 
…onfig/template.json

Co-authored-by: Damian Edwards <[email protected]>
@danmoseley @DamianEdwards
Update src/Aspire.ProjectTemplates/templates/aspire-xunit/.template.c…
cd38097 
…onfig/template.json

Co-authored-by: Damian Edwards <[email protected]>
@danmoseley @DamianEdwards
Update src/Aspire.ProjectTemplates/templates/aspire-xunit/.template.c…
11273e7 
…onfig/template.json

Co-authored-by: Damian Edwards <[email protected]>
@danmoseley @DamianEdwards
Update src/Aspire.ProjectTemplates/templates/aspire-starter/.template…
17bf0ee 
….config/template.json

Co-authored-by: Damian Edwards <[email protected]>
@danmoseley @DamianEdwards
Update src/Aspire.ProjectTemplates/templates/aspire-starter/.template…
b9605e0 
….config/template.json

Co-authored-by: Damian Edwards <[email protected]>
@danmoseley @DamianEdwards
Update src/Aspire.ProjectTemplates/templates/aspire-starter/.template…
22bdd4e 
….config/template.json

Co-authored-by: Damian Edwards <[email protected]>
@danmoseley @DamianEdwards
Update src/Aspire.ProjectTemplates/templates/aspire-starter/.template…
50c198b 
….config/template.json

Co-authored-by: Damian Edwards <[email protected]>
@DamianEdwards
Copy link
Member

@danmoseley can you drop a screenshot of the new options showing up in VS?

@danmoseley
Copy link
Member Author

image

@danmoseley
Copy link
Member Author

image

@DamianEdwards everything works now.

danmoseley
danmoseley commented Mar 10, 2025
View reviewed changes
src/Aspire.ProjectTemplates/templates/aspire-starter/.template.config/template.json Outdated
"description": "The version of xUnit.net to use.",
"displayName": "xUnit.net version",
"datatype": "choice",
"isEnabled": "(TestFramework == xUnit.net)",
Copy link
Member Author

Choose a reason for hiding this comment

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

'xunit.net' causes VS to crash. So template engine does not want quotes here, but does require them in the csproj.

Copy link
Member

Choose a reason for hiding this comment

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

@phenning thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

@danmoseley to be clear the templates are working now right? This option is only visible in VS when xUnit.net is the selected testing framework?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you attach a debugger and get a stack to see where the crash is coming from, or see if there is anything in the activity log?

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened issue linked below with stack. Incidentally the exception message (which isn't shown to the user) has a typo in it. I think it was something like "invalid chrcter" or similar.

LMK if you need more.

Copy link
Member Author

Choose a reason for hiding this comment

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

@DamianEdwards yes all this works, since I removed those single quotes. This should be good to merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

This option is only visible in VS when xUnit.net is the selected testing framework?

Oh, I misunderstood. Let me double check.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it doesn't - sorry. @phenning would you be able to tell me why the condition on this line (in its non crashing form) does not work?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, we tracked this down to a bug in the VS template engine- either it didn't work, or it crashes--
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2417642

To work around it, I've renamed some identifiers. Identifiers in some cases can't be substrings of other identifiers, until this is fixed.. @DamianEdwards fyi.

@danmoseley danmoseley marked this pull request as ready for review March 10, 2025 23:29
@danmoseley
Copy link
Member Author

https://developercommunity.visualstudio.com/t/quotes-in-project-templates-templatejs/10869029

DamianEdwards
DamianEdwards approved these changes Mar 12, 2025
View reviewed changes
Copy link
Member

@DamianEdwards DamianEdwards 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!

@danmoseley
Copy link
Member Author

this now seems to work as expected.

@danmoseley danmoseley closed this Mar 13, 2025
@danmoseley danmoseley reopened this Mar 13, 2025
@DamianEdwards DamianEdwards merged commit 393e254 into dotnet:main Mar 13, 2025
308 of 315 checks passed
@danmoseley danmoseley deleted the xunitv3 branch March 13, 2025 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phenning phenning phenning left review comments

Copilot code review Copilot Copilot left review comments

@DamianEdwards DamianEdwards DamianEdwards approved these changes

Assignees
No one assigned
Labels
area-templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update xUnit.net testing template to support xUnit.net v3
3 participants
@danmoseley @DamianEdwards @phenning