Enable more tests to run on all 3 runtimes, part 1 #10573
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
grendello
force-pushed
the
dev/grendel/test-runtimes
branch
from
3cce48b to
3d97d97
Compare
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs
Outdated
Show resolved
Hide resolved
|
|
||
| [Test] | ||
| public void CheckItemMetadata ([Values (true, false)] bool isRelease) | ||
| public void CheckItemMetadata ([Values (true, false)] bool isRelease, [Values] AndroidRuntime runtime) |
There was a problem hiding this comment.
I don't know if NUnit automatically finds all the enums for you, if you do: [Values] AndroidRuntime runtime
Let's see what the test run does, but I wonder if NUnit will siliently error and skip this test?
Or if it works fine, today I learned!
There was a problem hiding this comment.
If you decorate a bool or an enum parameter with [Values], it will enumerate all the possible values, yep :) Very handy
| IsRelease = true, | ||
| AotAssemblies = true, | ||
| IsRelease = isRelease, | ||
| AotAssemblies = aotAssemblies, |
There was a problem hiding this comment.
Maybe we should add a helper method for AotAssemblies, where you pass in the AndroidRuntime? And it returns true for Mono and false for the others.
There was a problem hiding this comment.
I was considering doing that, yeah, but then I decided that tests should be explicit as much as possible in what they do (even if it's repetitive/verbose), they should be treated as separate entities without hidden code that does something.
I made an exception in the IgnoreUnsupportedConfiguration helper method because what it does can change in the future and it would be a real shame to have to go through all the tests and change them individually.
Another exception to this, but justified I think, is the SetRuntime method when called for NativeAOT, that's just environment setup unrelated to any particular test as such, so I think it's fine.
| return false; | ||
| } | ||
|
|
||
| Assert.Ignore ($"NativeAOT: unsupported configuration (release == {release})"); |
There was a problem hiding this comment.
I believe the way NUnit works, Assert.Ignore() throws an exception that tells NUnit to skip.
So, I think you could make this method void and you don't need an if-return statement in each test.
There was a problem hiding this comment.
Yeah, Assert.Ignore() throws an exception, but I wanted to make the action explicit for human readers - thus the if-return pattern. It's verbose and repetitive, but makes it clear what's the intention and consequence. The actual action of ignoring via Assert.Ignore is hidden from the person reading a specific unit test. It is implied by the name, but without checking what IgnoreUnsupportedConfiguration actually does, they don't know the consequences.
Unit tests are a peculiar thing, and I think verbosity (and code repetition) are perfectly fine there, even desired.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…cross all 3 runtimes
…sts/BuildTest.cs Co-authored-by: Jonathan Peppers <[email protected]>
grendello
force-pushed
the
dev/grendel/test-runtimes
branch
from
3adffe6 to
98cb409
Compare
No description provided.