Skip to content

Conversation

@grendello
Copy link
Contributor

Fixes: #1400

@pjcollins
Copy link
Member

@grendello should (can?) we do this only when the linker is enabled? Perhaps this is something I can handle only when testing a project configuration that enables the linker? I am not entirely sure that we should be skipping the entire category due to one test failure that occurs only when linking.

@jonpryor
Copy link
Contributor

should (can?) we do this only when the linker is enabled?

We could wrap it in a #if RELEASE block, on the assumption that Release apps will be linked (fair?).

Alternatively we handle this in the "calling" code, e.g. by setting the $(ExcludeCategories)?

@grendello
Copy link
Contributor Author

should (can?) we do this only when the linker is enabled?

We could wrap it in a #if RELEASE block, on the assumption that Release apps will be linked (fair?).

Either that or we could narrow it down a bit more. Introduce the AOT macro and exclude the category only then. It all depends on how long do we think the situation will remain unchanged with the failing test. O opened the PR mostly to show how/where we can exclude categories :)

Alternatively we handle this in the "calling" code, e.g. by setting the $(ExcludeCategories)?

That should work too

@grendello grendello requested a review from jonpryor as a code owner April 23, 2018 20:43
@jonpryor jonpryor added the full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps) label Apr 24, 2018
@jonpryor
Copy link
Contributor

build

@jonpryor
Copy link
Contributor

Rebuilding with the full-mono-integration-build label set so that we build and run unit tests in the Release configuration, which should (hopefully) cause the assemblies to be linked. (They aren't linked in Debug).

If the linker is removing the method, shouldn't that happen whenever the linker runes, i.e. Release, not just Release+AOT? Let's find out!

@jonpryor
Copy link
Contributor

As I feared/suspected, the TaskSchedulerTests.GetTaskSchedulersForDebugger_ReturnsDefaultSchedule() test fails in Release configuration, w/o AOT: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/2971/testReport/Test%20collection%20for%20System.Threading.Tasks.Tests/TaskSchedulerTests/System_Threading_Tasks_Tests_TaskSchedulerTests_GetTaskSchedulersForDebugger_ReturnsDefaultScheduler___Release/

@grendello: instead of using #if AOT (and defining AOT in the first place), I think we should instead use #if !DEBUG.

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 3, 2018
Fixes: dotnet#1400

Related: dotnet#1421

The mono/corefx xUnit test
[`TaskSchedulerTests.GetTaskSchedulersForDebugger_ReturnsDefaultScheduler()`][0]
[fails when built in Release mode][1]:

[0]: https://github.com/dotnet/corefx/blob/893708a4c46047af6fb14bc0f2c33b3ca6adc527/src/System.Threading.Tasks/tests/TaskScheduler/TaskSchedulerTests.cs#L293-L301
[1]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/980/testReport/junit/Test%20collection%20for%20System.Threading.Tasks.Tests/TaskSchedulerTests/System_Threading_Tasks_Tests_TaskSchedulerTests_GetTaskSchedulersForDebugger_ReturnsDefaultScheduler___Release/

	System.NullReferenceException : Object reference not set to an instance of an object
	+++++++++++++++++++
	STACK TRACE:
	  at System.Threading.Tasks.Tests.TaskSchedulerTests.GetTaskSchedulersForDebugger_ReturnsDefaultScheduler ()

The cause is that the test grabs the `MethodInfo` for the
`TaskScheduler.GetTaskSchedulersForDebugger()` method, which the
linker removes -- correctly! as there are no references to it -- and
the test then attempts to *invoke* the `MethodInfo`, but the
`MethodInfo` is `null` (linked away!), resulting in the
`NullReferenceException`.

The test should be fixed, but in the meantime we can instead ignore
the xUnit *category* of this test when building for Release.
jonpryor added a commit that referenced this pull request May 3, 2018
Fixes: #1400

Related: #1421

The mono/corefx xUnit test
[`TaskSchedulerTests.GetTaskSchedulersForDebugger_ReturnsDefaultScheduler()`][0]
[fails when built in Release mode][1]:

[0]: https://github.com/dotnet/corefx/blob/893708a4c46047af6fb14bc0f2c33b3ca6adc527/src/System.Threading.Tasks/tests/TaskScheduler/TaskSchedulerTests.cs#L293-L301
[1]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/980/testReport/junit/Test%20collection%20for%20System.Threading.Tasks.Tests/TaskSchedulerTests/System_Threading_Tasks_Tests_TaskSchedulerTests_GetTaskSchedulersForDebugger_ReturnsDefaultScheduler___Release/

	System.NullReferenceException : Object reference not set to an instance of an object
	+++++++++++++++++++
	STACK TRACE:
	  at System.Threading.Tasks.Tests.TaskSchedulerTests.GetTaskSchedulersForDebugger_ReturnsDefaultScheduler ()

The cause is that the test grabs the `MethodInfo` for the
`TaskScheduler.GetTaskSchedulersForDebugger()` method, which the
linker removes -- correctly! as there are no references to it -- and
the test then attempts to *invoke* the `MethodInfo`, but the
`MethodInfo` is `null` (linked away!), resulting in the
`NullReferenceException`.

The test should be fixed, but in the meantime we can instead ignore
the xUnit *category* of this test when building for Release.
@jonpryor
Copy link
Contributor

jonpryor commented May 3, 2018

Superseded by PR #1630.

@jonpryor jonpryor closed this May 3, 2018
@grendello grendello deleted the issue1400 branch February 13, 2019 09:04
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants