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

Cleaning up AzDO Test Run Names #19575

Closed
wants to merge 4 commits into from

Conversation

analogrelay
Copy link
Contributor

I was spelunking around AzDO APIs and found that the test run names were a bit messy. Trying something to clean them up a little bit and see what we find.

@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Mar 4, 2020
@analogrelay analogrelay changed the title trying something to clean up the test run identifiers Cleaning up AzDO Test Run Names Mar 4, 2020
@analogrelay
Copy link
Contributor Author

Better, but not perfect:

image

@analogrelay
Copy link
Contributor Author

Going to look in to what I can do about the remaining run names. It's possible they come from Arcade though.

@analogrelay analogrelay force-pushed the anurse/clean-up-test-run-data branch from 980ed6a to 136deb4 Compare March 5, 2020 19:19
searchFolder: '$(Build.SourcesDirectory)/artifacts/TestResults/$(_BuildConfig)'
testRunTitle: $(AgentOsName)-$(BuildConfiguration)-xunit
mergeTestResults: true
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 know this is in eng/common which is not to be touched. I won't merge this, just testing something and will contribute to Arcade if it yields a good result.

@analogrelay
Copy link
Contributor Author

This looks much better. I'll need to make some Arcade changes to actually make it happen though.

image

@davidfowl
Copy link
Member

YESSSSSSSS please.

@analogrelay
Copy link
Contributor Author

YESSSSSSSS please.

Needs a change to dotnet/arcade (the changes in eng/common in this PR).

Copy link
Member

@MattGal MattGal left a comment

Choose a reason for hiding this comment

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

Seems reasonable and like a nice cleanup. I'd like to learn more about the lifespan and stateful-ness of the dump collector thing. if possible.

name: NetCorePublic-Pool
${{ if ne(parameters.isTestingJob, true) }}:
# Visual Studio Build Tools
queue: BuildPool.Server.Amd64.VS2019.BT.Open
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Unrelated to this PR, but please consider just using Enterprise for either build; since this was primarily in place to be a non-expiring SKU from before RTW, it'd be nice to eventually obsolete buildtools skus since we won't be able to ever obsolete enterprise and it should be able to build buildtools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only showing as edited because of the indentation change. I'd suggest filing a new issue to discuss that.

- ? ${{ if and(eq(parameters.agentOs, 'Windows'), eq(parameters.isTestingJob, true)) }}
: - powershell: ./eng/scripts/InstallProcDump.ps1
displayName: Install ProcDump
- powershell: ./eng/scripts/StartDumpCollectionForHangingBuilds.ps1 $(ProcDumpPath)procdump.exe artifacts/dumps/ (Get-Date).AddMinutes(160) dotnet
Copy link
Member

Choose a reason for hiding this comment

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

would like to learn more about this, such as what happens when the build ends (does it leave stuff running?) does it change machine state in any way that survives a reboot? does it install anything? , etc.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also only showing in this PR because of the indentation change. Probably something we should have a separate discussion about.

Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Curious…

agentOs: 'Windows'
buildArgs: ''
configuration: 'Release'
agentOs: "Windows"
Copy link
Member

Choose a reason for hiding this comment

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

Why switch to double-quotes? YAML supports either

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 fine either way. Wasn't intentional.

Copy link
Member

Choose a reason for hiding this comment

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

Prefer we left these alone to avoid unnecessary churn

timeoutInMinutes: ${{ parameters.timeoutInMinutes }}
cancelTimeoutInMinutes: ${{ parameters.cancelTimeoutInMinutes }}
? ${{ if and(eq(variables['System.TeamProject'], 'internal'), eq(parameters.agentOs, 'Windows'), eq(parameters.codeSign, 'true')) }}
: enableMicrobuild: true
Copy link
Member

Choose a reason for hiding this comment

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

Where is this syntax defined? It's not mentioned in https://docs.microsoft.com/en-us/azure/devops/pipelines/process/templates?view=azure-devops and the YAML 1.2 key / value syntax seems odd here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't changed in this PR, so 🤷‍♀

Copy link
Member

Choose a reason for hiding this comment

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

This was changed in this PR. See https://github.com/dotnet/aspnetcore/pull/19575/files#diff-f3d39efe7528a28e4995c18938c8a3d3L78

Seems like something is doing lots of extra changes to the YAML files you're touching, making it hard to see what's really changing.

@davidfowl
Copy link
Member

Can we get this merged? What’s left?

@analogrelay
Copy link
Contributor Author

Can we get this merged? What’s left?

We need to take the new arcade change in and someone needs to rebase and undo modifications to eng/common

@BrennanConroy
Copy link
Member

I'd like us to fix up the js test names too

@analogrelay
Copy link
Contributor Author

analogrelay commented Apr 1, 2020

We can do that in a separate PR. This change should fix the top-level Test Run names, but if you want to fix the individual test names, we'll have to do that in the JS tests themselves.

@davidfowl
Copy link
Member

Lets get it merged then. This one right dotnet/arcade#5171?

@analogrelay
Copy link
Contributor Author

Is this still relevant @davidfowl @Pilchie ?

@Pilchie
Copy link
Member

Pilchie commented Jun 23, 2020

I think this is still relevant. Going to close/reopen to get another run.

@Pilchie Pilchie closed this Jun 23, 2020
@Pilchie Pilchie reopened this Jun 23, 2020
@analogrelay
Copy link
Contributor Author

With the Arcade PR now merged, it should be ready to go. I just don't have access to AzDO to see if it's working 😆

@BrennanConroy BrennanConroy marked this pull request as ready for review June 24, 2020 04:01
@BrennanConroy
Copy link
Member

Ah, merge conflicts

@jkotalik
Copy link
Contributor

jkotalik commented Nov 5, 2020

@Pilchie should we delegate finishing this PR to someone else? @BrennanConroy do you have more context?

@jkotalik
Copy link
Contributor

jkotalik commented Nov 5, 2020

Is this just renaming queues to more clear names?

@BrennanConroy
Copy link
Member

Is this just renaming queues to more clear names?

Basically, see the screenshots above.

@Pilchie
Copy link
Member

Pilchie commented Nov 5, 2020

That would be great. My yaml isn't great, and these files have changed enough that I had a hard time working through the conflicts here. Any volunteers @BrennanConroy / @dotnet/aspnet-build / @dotnet/aspdoi ?

@BrennanConroy
Copy link
Member

Sure, I've complained about these names before and have done minor attempts at fixing them in the past. I'll clean this up

@BrennanConroy
Copy link
Member

Thanks Andrew, closing in favor of #27560 (which is mostly just a copy of this PR)

@BrennanConroy BrennanConroy deleted the anurse/clean-up-test-run-data branch November 6, 2020 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants