robotdotnet / WPILib

DotNet implementation of WPILib for FIRST Robotics Competition (FRC)
27 stars 8 forks source link

Made tests that use System.Reflection.Context and the System.Reflection.Context reference conditionally not compile for the Travis CI build (since Mono doesn't have that assembly). #101

Closed jkoritzinsky closed 8 years ago

jkoritzinsky commented 8 years ago

This pull request will fix issue #99. I decided to have the code conditionally compile out instead of removing it because I want to leave possible support in the case that we ever complete #100 (where we will be able to utilize the functionality).

codecov-io commented 8 years ago

Current coverage is 36.90%

Merging #101 into master will increase coverage by +1.29% as of fd90591

@@            master   #101   diff @@
=====================================
  Files          121    121       
  Stmts         7951   7969    +18
  Branches       985    987     +2
  Methods          0      0       
=====================================
+ Hit           2832   2941   +109
- Partial        146    164    +18
+ Missed        4973   4864   -109

Review entire Coverage Diff as of fd90591

Powered by Codecov. Updated on successful CI builds.

jkoritzinsky commented 8 years ago

The test failure on the pr check is not related to any code from this build... Not quite sure what happened there. @ThadHouse can you take a look?

ThadHouse commented 8 years ago

That test fails semi often if the AppVeyor servers are more loaded up. I might just cut it.

Also, if CustomReflectionContext doesn't exist in Mono, how would you actually use the ReflectionContext stuff on the RoboRIO? Is there a way to do so that we know works on the RoboRIO?

EDIT: AHH didnt read the entirety of the PR. Maybe we should comment it out of the current code for now, if there's no way to run it on the RoboRIO. That way it doesn't get accidentally used where it doesnt work.

ThadHouse commented 8 years ago

Also maybe we should rename the Travis config to the Mono config. The only reason I found the bug is because someone tried compiling manually on Mono and it didn't work.

jkoritzinsky commented 8 years ago

There isn't a way at the moment but it's easier to leave the code in now then remove it and re-add it when Mono support is added. This way the tests make sure that it doesn't break (at least on AppVeyor).

jkoritzinsky commented 8 years ago

I can rename it to Mono if you want. I was just naming it similar to the AppVeyor target since it is also used for CI.

ThadHouse commented 8 years ago

Ah OK. Should we mark it obsolete and give a decent warning message so nobody attempts to use it. I know thats kind of a backwards way of using obsolete, but unless theres something else it might be the best way. Its gonna be a weird error if somebody tries to load that onto a robot.

jkoritzinsky commented 8 years ago

Well the Extras assembly should still build. There's just nothing useful (other than null) that can be passed as a parameter for a ReflectionContext.

ThadHouse commented 8 years ago

The only reason for the AppVeyor specific one is to ignore all the compiler warnings. It doesn't actually change any configuration. Release will build properly. However as release won't build properly on mono anyway, it might be worth it to name it Mono.

ThadHouse commented 8 years ago

It will build, however if building for the RoboRIO in Visual Studio, adding a custom reflection context would compile correctly. Just when it gets uploaded to the RoboRIO it would crash.

jkoritzinsky commented 8 years ago

Oh. Got it. I'll make the change and push it on to this branch in the next few days.

jkoritzinsky commented 8 years ago

I have the configuration named MonoDebug now. Let me know what you think.

ThadHouse commented 8 years ago

Looks good other than the Travis configuration either needs to change the test location to bin/MonoDebug, or switch the MonoDebug configuration back to Output\ Other then that should be good.

msoucy commented 8 years ago

Is there a way to get this completed? It would be nice to have on the mainline

jkoritzinsky commented 8 years ago

Sorry for the delay. I'll merge this in as soon as the CI build confirms that I haven't broken anything.