-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix the only ilproj test that uses multiple command-line variants #61750
Conversation
I believe that despite its singular nature this is worth a separate review to make sure we're in agreement regarding the conversion principles. There are several things worth noting here: 1) According to the current plan we're continuously documenting in dotnet#54512 we want to remove command-line parameters from all test entrypoints. Variant tests driven by command-line parameters should be turned into multiple test cases marked with separate [Fact] attributes. 2) For ilproj tests, to facilitate local debugging, our current plan is to keep them runnable as standalone executables. This implies that ilproj tests comprising several [Fact] test entrypoints require a new entrypoint that just calls into the individual test cases. 3) The Roslyn-generated merged wrapper for the tests won't care about the "composite" main (that is for local debugging only), it will directly identify and use the individual test cases marked with [Fact] attributes. 4) In accordance with this scheme, such composite ILPROJ tests are specific in not having their entrypoint method itself marked with the [Fact] attribute. 5) Funnily enough this example nicely demonstrates the implied cleanup - the entire command-line machinery is only used for a handwritten switch to choose one of the three variants; moreover we only exercised two out of the three variants, possibly due to an authoring bug when creating the variant test, potentially caused by previous complexity of such endeavor. Thanks Tomas
Tagging subscribers to this area: @hoyosjs Issue DetailsI believe that despite its singular nature this is worth a separate
Thanks Tomas
|
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
IL_0021: call int32 [System.Runtime]System.Array::IndexOf<string>(!!0[], !!0) | ||
IL_0026: ldc.i4.m1 | ||
IL_0027: beq.s IL_0031 | ||
IL_0037: ldc.i4.s 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IL_0037
label definitely doesn't belong here - but I think it's okay and not worth trouble of re-doing ilasm/ildasm round trip to get them right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, thanks Egor for your feedback, as you can certainly see I just manually copied over the different sections of the previously main IL to the individual methods. This one is definitely a singleton but there are some tests that specify command-line arguments even though they don't refer to "base tests" to run, I'm investigating now what exactly happens there and it may have further implications on this aspect of il test refactoring.
I believe all outstanding issues are unrelated to my change, merging in. Please let me know if you believe additional work is needed here. |
I believe that despite its singular nature this is worth a separate
review to make sure we're in agreement regarding the conversion
principles. There are several things worth noting here:
According to the current plan we're continuously documenting in
CoreCLR test suite optimization proposal: support for test project grouping #54512 we want to remove
command-line parameters from all test entrypoints. Variant tests
driven by command-line parameters should be turned into multiple
test cases marked with separate [Fact] attributes.
For ilproj tests, to facilitate local debugging, our current plan
is to keep them runnable as standalone executables. This implies that
ilproj tests comprising several [Fact] test entrypoints require a
new entrypoint that just calls into the individual test cases.
The Roslyn-generated merged wrapper for the tests won't care about
the "composite" main (that is for local debugging only), it will
directly identify and use the individual test cases marked with
[Fact] attributes.
In accordance with this scheme, such composite ILPROJ tests are
specific in not having their entrypoint method itself marked with
the [Fact] attribute.
Funnily enough this example nicely demonstrates the implied
cleanup - the entire command-line machinery is only used for a
handwritten switch to choose one of the three variants; moreover
we only exercised two out of the three variants, possibly due to
an authoring bug when creating the variant test, potentially caused
by previous complexity of such endeavor.
Thanks
Tomas