testC3DFileAdapter failing on AppVeyor: "Unable to load 'walking2.c3d' within 100.000000ms."

[tkuchida]

C3DFileAdapter 'walking2.c3d' loaded in 167ms
testC3DFileAdapter FAILED: Unable to load 'walking2.c3d' within 100.000000ms.

https://ci.appveyor.com/project/opensim-org/opensim-core/branch/master/job/9cqyblh302drdukf#L245

See testC3DFileAdapter.cpp#L78-L81:

opensim-core/OpenSim/Common/Test/testC3DFileAdapter.cpp

Lines 78 to 81 in 5a59cd7

	// The walking C3D files included in this test should not take more
	// than 40ms on most hardware. We make the max time 100ms to account
	// for potentially slower CI machines.
	const double MaximumLoadTimeInMS = 100;

From PR #2198. @aseth1

[aseth1]

I agree this is annoying especially since we had no issues with it failing initially. The purpose of this condition was to avoid slowdowns creeping back in without us knowing. If we need to make this condition 200ms or longer to pass on CI I can do that, but then the test may be less effective. The original C3D reading code was taking several seconds, so perhaps even 200ms would still be better than nothing. I am open to suggestions.

[tkuchida]

It would be nice if this failure didn't cause to appear on the home page. The test condition is an absolute metric (within 100ms); perhaps it should be relative (e.g., within some multiple of reading another file) since we can't predict latency on the CI computers---or on the computer of a user who is building from source?

[aseth1]

perhaps it should be relative (e.g., within some multiple of reading another file) since we can't predict latency on the CI computers

Great idea! I'll try to swing this.

[aymanhab]

The issue reappeared on AppVeyor, the timing test fails in one configuration and not the other! Obviously we can't control how ci servers prioritize jobs. I'm hesitant to double the time in general but maybe we can double it on appveyor machines only since these seem to be much slower. Other thoughts @chrisdembia @tkuchida ?

[tkuchida]

@aymanhab The conversation continues in #2218 and #2221. I have the same suggestion now as on Jul 18, 2018: "Perhaps the wall clock speed test should be skipped on CI?"

[aymanhab]

Since we don't have this granularity, the options are to skip test altogether on CI (I prefer not to do it since it's the only test that uses the BTK library), or just comment out the timing section which will be removed on all platforms. I'm leaning to the latter since measuring timing on ci is rather fragile anyway.
@chrisdembia would love to hear your thoughts as well.

[tkuchida]

Since we don't have this granularity, the options are to skip test altogether on CI (I prefer not to do it since it's the only test that uses the BTK library), or just comment out the timing section which will be removed on all platforms.

Could also move just this timing test into a separate test file that is skipped on CI but run locally 🐫

[aymanhab]

I thought about that too @tkuchida but didn't like the code duplication 🦆 🦆

[chrisdembia]

I think we simply introduce an environment variable into the test to control whether or not the timing is run.