Skip to content

Conversation

@zivkan
Copy link
Member

@zivkan zivkan commented Oct 22, 2020

Bug

Fixes: NuGet/Home#10169
Regression: Yes (but didn't ship)

  • Last working version:
  • How are we preventing it in future:

Fix

Details: Add new capability check just for PackageReferences, and improve xmldoc comments for existing capability checks to explain what they do and do not tell us.

Testing/Validation

Tests Added: No
Reason for not adding tests: I'm still looking into how to test this. I'm not sure if we can mock the VS project adapter, but having written this I think we can, which means I probably need to modify this PR to add tests.
Validation:

@zivkan zivkan requested a review from a team as a code owner October 22, 2020 00:47
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

👏

Can we add an sfproj test so we don't regress this in the future? :D
In the interest of getting it merged earlier, I think it can be done in a future PR.

@zivkan zivkan requested a review from nkolev92 October 22, 2020 20:42
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

LGTM :)

Still targetting P1 QB mode?

@zivkan
Copy link
Member Author

zivkan commented Oct 22, 2020

I was too slow to post this before you reviewed again.

I think it might be difficult to create a E2E or Apex test to create a real sfproj project. Maybe I'm wrong and it's no more difficult than any other project, but when manually creating a Service Fabric project, it creates the sfproj, which is a packaging project, but the wizard also creates a service, for the sfproj to package. This second wizard screen has multiple options, so I don't know how that would work in Apex. Maybe it's easier in E2E, but I don't like E2E tests and think we should move entirely to Apex tests.

I created a unit test, although it's a bit messy to make testable. Yes, there's a risk that this test could pass, but real projects still fail. For now I think it's good enough.

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.

Fix sfproj support

3 participants