-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Conversation
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. |
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. |
610c374
to
da872b7
Compare
@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.) |
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. |
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 :) |
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. |
Good catch @naricc -- before coreclr tests were run always that there was a change in runtimetests cause it was in the coreclr subtree. |
da872b7
to
4a22294
Compare
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. |
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