Skip to content

Don't reference test asset projects in Build.props #39336

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
Merged
merged 1 commit into from
Jan 6, 2022
Merged

Don't reference test asset projects in Build.props #39336

merged 1 commit into from
Jan 6, 2022
+23 −15

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented Jan 6, 2022

  • test projects should include any necessary references
    • should further reduce build ordering problems
    • also, removes mention of a few non-existent folders
  • but, special case a few test asset projects
    • reference three used in test classes in relevant test projects
    • reference five that are otherwise unreferenced in Build.props
  • move src\Components\Web.JS\node_modules mention to correct part of Build.props

nit:

  • sort "not meant to be built" project list

@dougbu
Don't reference test asset projects in Build.props

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
11d1506 
- test projects should include any necessary references
  - should further reduce build ordering problems
  - also, removes mention of a few non-existent folders
- but, special case a _few_ test asset projects
  - reference three used in test classes in relevant test projects
  - reference five that are otherwise unreferenced in Build.props
- move `src\Components\Web.JS\node_modules` mention to correct part of Build.props

nit:
- sort "not meant to be built" project list
@dougbu dougbu added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jan 6, 2022
@dougbu dougbu requested a review from halter73 as a code owner January 6, 2022 02:16
dougbu
dougbu commented Jan 6, 2022
View reviewed changes
eng/Build.props
Comment on lines +21 to +25
Exclude="$(RepoRoot)src\Components\WebAssembly\testassets\WasmLinkerTest\*.*proj;
$(RepoRoot)src\Components\WebView\Samples\PhotinoPlatform\testassets\PhotinoTestApp\*.*proj;
$(RepoRoot)src\Http\Routing\test\testassets\RoutingSandbox\*.*proj;
$(RepoRoot)src\Security\Authentication\Negotiate\test\testassets\Negotiate.Client\*.*proj;
$(RepoRoot)src\Security\Authentication\Negotiate\test\testassets\Negotiate.Server\*.*proj;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BrennanConroy @captainsafia @davidfowl @JamesNK @javiercn @pranavkm @SteveSandersonMS @Tratcher you either created or updated the above projects at some point. Are all of them still relevant i.e. important to build in the CI even though no tests use them❔ I suspect the answer is "yes" for the two Negotiate projects but am doubtful about the other three.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put another way, I'd rather move or remove these projects than act as if they are test assets. Please let me know…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/ping

Does anyone know the current status of WasmLinkerTest.csproj, PhotinoTestApp.csproj, and RoutingSandbox.csproj❔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't actually mean to merge this yet☹️ Will leave it alone and we can clear out or move those projects in a follow-up if that's appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should keep building the Negotiate projects, if only to catch build breaks.

@dougbu
Copy link
Contributor Author

dougbu commented Jan 6, 2022

/fyi I checked that all other projects in testassets/ folders are referenced (at least indirectly) in test projects that use them.

@sebastienros
Copy link
Member

/AzurePipelines help

@azure-pipelines
Copy link

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@dougbu
Copy link
Contributor Author

dougbu commented Jan 6, 2022

What's up @sebastienros❔

@sebastienros
Copy link
Member

I chose your PR to test the feature, you should be proud.

@wtgodbe
Copy link
Member

wtgodbe commented Jan 6, 2022

/azp where

@azure-pipelines
Copy link

Azure DevOps orgs getting events for this repository:

@dougbu
Copy link
Contributor Author

dougbu commented Jan 6, 2022

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@sebastienros
Copy link
Member

/azp list aspnetcore-*

@azure-pipelines
Copy link

No pipelines found for this repository.

@dougbu
Copy link
Contributor Author

dougbu commented Jan 6, 2022

Interesting nothing but run is documented in the "additional info" link😺

Trying again because list was incomplete…

/azp list

@dougbu
Copy link
Contributor Author

dougbu commented Jan 6, 2022

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@dougbu dougbu merged commit 54ff379 into dotnet:main Jan 6, 2022
@dougbu dougbu deleted the dougbu/cleanup.build.props branch January 6, 2022 23:05
@ghost ghost added this to the 7.0-preview1 milestone Jan 6, 2022
@dougbu dougbu mentioned this pull request Jan 19, 2022
Open
This was referenced Jan 24, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wtgodbe wtgodbe

@davidfowl davidfowl

@pranavkm pranavkm

@JamesNK JamesNK

@SteveSandersonMS SteveSandersonMS

@Tratcher Tratcher

@captainsafia captainsafia

@javiercn javiercn

@BrennanConroy BrennanConroy

@halter73 halter73

Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Milestone
7.0-preview1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dougbu @sebastienros @wtgodbe @Tratcher