Test merging IL_Conformance #80597
Test merging IL_Conformance #80597
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-reflection-metadata Issue DetailsThis is a first draft of what would be repeated over each leading subdirectory in src/tests/JIT. I followed the same steps @markples is doing based on @trylek ILTransform tool. It is easier to review each commit separately. Each commit name is the command used with its params. I am able to build the locally, but tests are not allocated under I guess these tests are run because I can see there are some being excluded but I cannot find a *.proj file referencing the ilproj files inside IL_Conformance as I saw on trylek pr. Do you know @trylek how these tests are run? This is the only reference to IL_Conformance I can find, and it seems similar to the leading references to specific Methodical tests, so I guess we shouldn't remove it. Any opinions on that?
|
|
I'm pretty sure that a missing Your test runs have enough to indicate at least one fix - check out the logs. The |
Thanks @markples, you are right and it is needed the -priority=1 and use the tree switch (not dir)
Thanks, I fixed those. The leading errors seems to not be related to this pr. Can I schedule priority 1 tests with these failures? |
|
/azp run outerloop |
|
No pipelines are associated with this pull request. |
I assume you mean test errors/timeouts on some platforms, in which case you'll be fine with outerloop. It should just be the obvious dependencies that would block it (if you can't build the runtime or can't build the tests, then you can't run them). |
|
/azp list |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -17,12 +17,12 @@ | |||
| //----------------------------------------------------- | |||
|
|
|||
|
|
|||
| .assembly 'add'{ | |||
| .assembly 'add_'{ | |||
There was a problem hiding this comment.
I think something may have gone wrong in the dedup renaming. I don't think this and others need to be renamed at the project/assembly level, which will add a lot of churn to the test source history. Only the namespace should need the rewrite.
(old\base\add.il is the only add.il, but old\base\add.il, old\conformance_base\add_i.il, and others all have namespace 'add')
There was a problem hiding this comment.
IIRC the only limitations are that the assembly name must match the project name and that assembly names must be unique within a single merged test wrapper, otherwise we cannot generate functional C# code calling their entrypoints - in the former case (assembly vs. project name mismatch) the Roslyn generator at some point thinks that the project (dll) name is the assembly name and uses that for module qualification of the method call which subsequently fails at runtime because runtime compares it with the logical assembly name. In the latter case (assembly names unique within a test wrapper) this is due to the fact that all the test assemblies are ultimately copied to the merged test wrapper output folder so same-named assemblies would stomp on each other; this is "silent" in the sense that nothing fails but you'll see a lower number of tests compared to the non-merged version. A lower number of tests executed by src\tests\run can also indicate premature termination of the merged test wrapper that happened to me for several JIT/Methodical tests that were using Environment.Exit to report success.
There was a problem hiding this comment.
This is the most annoying for silly recursive tests that need to exit early, in a handful of cases I had to do non-trivial rewrites for this reason. IIRC I typically introduced a static variable representing the exit code, changed the code calling Environment.Exit to set that instead, patched the recursive descents to check the variable prior to the call and the main method to use the variable for the final return value. There were less than half a dozen cases like these so I guess each of them is unique, feel free to devise your own method to overcome the Environment.Exit problem.
There was a problem hiding this comment.
You are right, I was matching namespaces with il files, but it is no need.
There was a problem hiding this comment.
I'm pretty sure that there is something required at the namespace level (or more specifically that type names need to be unique), but I'm not sure exactly what the restriction is. My original understanding (I think from ILTransform code) was that this applied to the type containing the test entrypoint (perhaps due to invoking it without assembly scoping?). However, I then hit build errors for types that weren't the container for test entrypoints, so I adjusted ILTransform to look at all of the type names.
It is quite possible that I misinterpreted what was going on.
That said, deduplicating the types will be necessary if we try to combine multiple tests into one C# compiler invocation with many files (as a future consolidation step to save time). Perhaps this could be relevant on the IL side though the tool invocation might be trickier.
We have ILTransform scanning for Environment.Exit, adding RequiresProcessIsolation for them, and reporting them in the log. I think we've been pretty lucky that they are rare / non-existent across jit\regression / il_conformance, but we will likely hit this again.
There was a problem hiding this comment.
Sounds great. IIRC @jkoritzinsky was looking into that at some point; there's some hacky partially supported form of syntax that lets you address the same-typed name in multiple assemblies via some obscure form of qualification, however @jkotas suggested it would be better to clean these cases up so that's what I did, typically in the Loader/classloader/TypeGeneratorTests all tests were using the same namespace and class name i.o.w. the fully qualified entrypoint name was the same for all of them, I believe that was the point of this particular deduplication.
|
Running This is what I get from this PR without the wrapper merge commit: And now the state of art of the PR: |
|
The 3 tests inside JIT.IL_Conformance.IL_Conformance (TestConvertFromIntegral, AutoInit and conv_ovf_i1_un ) are included in JIT.IL_Conformance.XUnitWrapper.dll |
|
I can see all the test dlls under IL_Conformance being generated and placed under |
Removing the tests it runs does not help running others. So there is no particular problem running those tests. Any guess of what could be @trylek ? How would you debug this? |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsThis is a first draft of what would be repeated over each leading subdirectory in src/tests/JIT. I followed the same steps @markples is doing using @trylek ILTransform tool, this is my version of the tool. It is easier to review each commit separately. Each commit name is the command used with its params. I am able to build the locally, but tests are not allocated under I guess these tests are run because I can see there are some being excluded but I cannot find a *.proj file referencing the ilproj files inside IL_Conformance as I saw on trylek pr. Do you know @trylek how these tests are run? This is the only reference to IL_Conformance I can find, and it seems similar to the leading references to specific Methodical tests, so I guess we shouldn't remove it. Any opinions on that?
|
|
After adding the |
|
I reverted all il/ilproj/assembly renames removing the '_' that should have been only for namespaces. All tests are still reachable. |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
115324f to
aecff40
Compare
|
It seems that now merging to main would run the tests in the legacy and the merged way. I have not realized why yet. |
|
Hmm, at the very least the encouraging news is that running the tests through the legacy wrapper takes about 12 times longer than in the merged version. The legacy wrappers are based on locating cmd / sh test execution scripts; for out-of-process merged tests that do produce execution scripts we produce a special file with the extension |
|
I'm wondering where you're getting this table from as the runs haven't finished yet. If it's from your local testing, IIRC there's a weird catch that you need to delete the folder |
|
Just to clarify, this only applies to local innerloop of developers actively working on test merging so that at some point they have the "non-merged" xUnit summary files in their |
|
The bottom line is that outputting stable-named test results into the logs folder along with "normal" timestamped logs is a very hacky thing to do. |
This is the result of running |
@trylek My wrong, it seems it is running only the new tests so probably happened what you said before. Thanks! |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Copying to the PR - test execution is 4 seconds. (This opens a new question as to whether this is worth an entire helix job, but that is beyond the scope of this PR.) |
Should we follow up this on a new or related issue? |
|
Related issue #71732 |
|
@BrianBohe - are you saying that the entire 400-something test suite takes just 4 seconds to execute? That would be more optimistic than I'd have hoped. Anyway, the entire Pri1 file is just about 10500 tests or so, that would mean approximately 25 Helix work items this size, I don't think we need to squash more tests into a single merged test. After all please remember that in stress modes they'll take 10~100 times longer, the more tests the more hassle when a test crashes, potential for one test to affect a later test via GC allocation or whatnot, we're not merging the tests because that would make running them easier, we're doing it solely to reduce times and costs and it has its own cost. My gut feeling is that several hundred tests in a single merged wrapper is more than enough. @BruceForstall has actually been pushing for finer-grained split with fewer test per wrapper as that would allow for better Helix work balancing in the super-slow GC stress tests. |
|
For the tests I merged previously, IIRC JIT/Methodical comprise about 2K tests and use five wrappers i.e. approximately 400 tests each, the TypeGeneratorTests use 15 wrappers 100 tests each (the last wrapper actually has 101 tests as there are 1501 tests total). |
|
4 seconds is plausible for il_conformance, as it doesn't do very much. For example, base\add is ~50 straight-line IL instructions. I suppose small repros for specific JIT bugs will be similarly quick, but then "real" tests will be much slower. Perhaps in the future we can be more sophisticated with groupings, but I agree that we don't need to do anything now or track anything. |
|
Sounds good. When I worked with Jeremy on xUnit reporting for merged tests by the end of 2021, I had to increase the number of decimal digits from 3 to 6 because it turned out that some tests are now taking something like 50 microseconds, such a thing was naturally not observed before the merging. |
This is a first draft of what would be repeated over each leading subdirectory in src/tests/JIT. I followed the same steps @markples is doing using @trylek ILTransform tool, this is my version of the tool. It is easier to review each commit separately. Each commit name is the command used with its params.
I am able to build the locally, but tests are not allocated under
artifacts/tests/coreclr/<os>.<arch>.<build_type>/so probably I am missing something. I am opening this pr to see whether some pipeline is running them.I guess these tests are run because I can see there are some being excluded but I cannot find a *.proj file referencing the ilproj files inside IL_Conformance as I saw on trylek pr. Do you know @trylek how these tests are run? This is the only reference to IL_Conformance I can find, and it seems similar to the leading references to specific Methodical tests, so I guess we shouldn't remove it. Any opinions on that?