Skip to content

July infra rollout: Move runtime tests out of the coreclr folder #37995

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

Closed
wants to merge 0 commits into from

Conversation

trylek
Copy link
Member

@trylek trylek commented Jun 16, 2020

This is the change #37977 rebased against runtime master. It is currently unclear whether we want to merge this into dev/infrastructure, immediately into master or merge this into master in three weeks (as of the July infra rollout on 7/6). The main argument against immediate merge into master is that this is exactly the type of large and potentially disruptive change we designed the infra rollout cadence for; the argument for the merge is that technically the change affects neither local developer workflow nor YAML pipelines and any form of its maintenance against the ever-changing CoreCLR test set for three weeks (whether in the form of a PR against master or merged into dev/infrastructure) is going to be a pain. While I selfishly agree with the pro arguments, I feel too biased to realistically assess the end developer impact / annoyance so I'm hoping for a broader discussion on this topic.

Thanks

Tomas

Fixes: #37440

/cc: @dotnet/runtime-infrastructure

@trylek
Copy link
Member Author

trylek commented Jun 16, 2020

One upside is that I have intentionally kept the internal structure of the change split into the initial mechanical move and follow-up script changes; so that, even if we decide to put a hold on the change, it's going to be relatively easy to reconstruct it right before the rollout by combining a fresh migration of the test folder with the remaining commits cherry-picked off this change.

The downside is that the change calls for a mop-up cleanup moving the remaining parts of Common, consolidating some of the Directory.Build.props / targets scripts and migration of additional test build scripts under the new src/tests folder but that will require counterpart YAML changes and make maintenance in a single PR a real nightmare.

@safern safern added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 16, 2020
@safern
Copy link
Member

safern commented Jun 16, 2020

Thanks @trylek -- I added the NO MERGE label since there is the question of if we should merge it right away or wait for the infra rollout.

@trylek trylek force-pushed the CoreCLRTestMoveInMaster branch from 610c374 to da872b7 Compare June 16, 2020 23:22
@jkotas
Copy link
Member

jkotas commented Jun 16, 2020

@BruceForstall The codegen team is probably most affected by this. Are you ok with merging this bulk move now, or would you prefer this to be merged in two weeks during July infrastructure changes rollout window? (The discussion of pros and cons of each option is above.)

@trylek
Copy link
Member Author

trylek commented Jun 16, 2020

Thanks Santi for your feedback. I have rebased the change against the latest master and I have squashed the script change commits to simplify longer-term maintenance as for now I assume that, based on your feedback, this change is going to be merged directly into master during the July infra rollout (on 7/6 i.e. in 20 days). I am fine with this and as part of the review I'm open to suggestions for minor improvements of the change including migration of other parts of the Common folder but I want to avoid making any YAML-sensitive changes or changes affecting developer workflow as part of this PR.

@naricc
Copy link
Contributor

naricc commented Jun 17, 2020

Just making the same comment here I made on the other PR before the rebase:

I think we need to update the runtimetests subset in runtime.yml, since it refers to the test path: https://github.com/trylek/runtime/blob/6ca9f624c774b613d96d29e786881b1cd772f5ff/eng/pipelines/runtime.yml#L112

We also probably want to change it so the coreclr tests are run if the runtimetest path is updated.

Although that would kind of undermine your argument that it doesn't effect yml pipelines :)

@BruceForstall
Copy link
Contributor

@BruceForstall The codegen team is probably most affected by this. Are you ok with merging this bulk move now, or would you prefer this to be merged in two weeks during July infrastructure changes rollout window? (The discussion of pros and cons of each option is above.)

I don't have a strong feeling either way, as long as the timeline is well communicated (internal email, Teams, etc.). Also, I would like several or even many of the JIT stress AzDO pipelines to run successfully on the PR, so we don't end up with a long tail of AzDO issues. Also, because of that, I'd prefer it happen early in the week so we don't accidentally break important once-a-week weekend runs.

@safern
Copy link
Member

safern commented Jun 17, 2020

Good catch @naricc -- before coreclr tests were run always that there was a change in runtimetests cause it was in the coreclr subtree.

@trylek trylek changed the title Move runtime tests out of the coreclr folder (against master branch) July infra rollout: Move runtime tests out of the coreclr folder Jun 17, 2020
@trylek trylek closed this Jun 17, 2020
@trylek trylek force-pushed the CoreCLRTestMoveInMaster branch from da872b7 to 4a22294 Compare June 17, 2020 22:58
@trylek
Copy link
Member Author

trylek commented Jun 17, 2020

Hmm, please rest assured that I haven't accidentally merged and closed the PR, I just messed something up with regards to the branch content so that GitHub sees it as empty right now. I'm working on fixing this, I'll just need to double-check whether I can revive this PR or whether I'll need to open another one, I hit some issues the last time a similar thing happened to me.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-coreclr NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

July infra rollout: move tests shared between coreclr and mono from src/coreclr/tests/src to src/tests
5 participants