Display time component of datetime

[lognaturel]

Closes #282
Closes #680

What has been done to verify that this works as intended?

Newly-added tests.

Why is this the best possible solution? Were any other approaches considered?

This is the least-invasive, least-risky approach I could think of for both the fix and the tests. In the fullness of time it would be great to refactor this date/time code. For example, LocalDate would be a better underlying type for DateData. Ideally we wouldn't have to set the global time and timezone to write tests. But I think it makes sense to add more testing of this kind first before taking on a bigger refactor.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This is intended to be very narrow and just display the time component for datetime values used in outputs. This matches Enketo and Web Forms and makes it easier to compare behavior between the 3.

Do we need any specific form for testing your changes? If so, please attach one.

now.xlsx is a good one to illustrate why we're making the change.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

[seadowg]

Did you actually run into problems that motivated the changes to this file. That would confuse me for two reasons:

1. maxParallelForks should default to 1
2. maxParallelForksbeing greater than 1 should spawn separate process for each test "fork" - messing with state static state (like with TimeZone.setDefault) should not be an issue

Maybe I'm missing a detail here?

[lognaturel]

Good question, should have written about this in the PR description.

I briefly explored whether I could find a way not to modify global state but anything I could come up with was going to be a big change. I felt somewhat sheepish about changing global state and my understanding is that there are some cases like killing a test in which @After might not run. So I thought isolating that test class could offer greater protection against that edge case. Really what I wanted was JUnit 5's @Isolated tag but we use JUnit 4.

When I read about how to isolate test classes in JUnit 4, I saw examples with explicit maxParallelForks of 1 but you're right that since we're talking about forked JVM processes that doesn't seem like it would help anything.

I'm taking all of that attempt at blocking parallelization out for now. I expect to follow up with more tests that modifying timezone and time and I'll see what kind of protection is needed then if applicable.

[seadowg]

I briefly explored whether I could find a way not to modify global state but anything I could come up with was going to be a big change. I felt somewhat sheepish about changing global state and my understanding is that there are some cases like killing a test in which @after might not run.

The only cases where @After won't run is if the test process itself is killed. For modifying static state, this doesn't any reprecussions (the process doesn't exist so there's not state to mess up). The only time you need to worry about it is when you're modifying state that exists outside the test process. An example would be test setup/teardown that mess with animation speeds in Android emulator.

Generally, when messing with time you want to avoid static state by just treating it like any other dependency to the logic under test (which it is), but obviously reworking all of JavaRosa to take time as an input would be a big job to block this fix on.

For the moment, the biggest risk with tests like this that do static static setup/teardown is messing up that setup/teardown code. The best way to avoid that in my opinion is to encapsulate it in a JUnit @Rule so that it's contained and reusable. This is a perfect use case for subclassing ExternalResource.

[seadowg]

#854 (comment)