[CORE] [TEST] Fix SimpleDateParamTest

[scwf]

sbt.ForkMain$ForkError: 1424424077190 was not equal to 1424474477190
    at org.scalatest.MatchersHelper$.newTestFailedException(MatchersHelper.scala:160)
    at org.scalatest.Matchers$ShouldMethodHelper$.shouldMatcher(Matchers.scala:6231)
    at org.scalatest.Matchers$AnyShouldWrapper.should(Matchers.scala:6265)
    at org.apache.spark.status.api.v1.SimpleDateParamTest$$anonfun$1.apply$mcV$sp(SimpleDateParamTest.scala:25)
    at org.apache.spark.status.api.v1.SimpleDateParamTest$$anonfun$1.apply(SimpleDateParamTest.scala:23)
    at org.apache.spark.status.api.v1.SimpleDateParamTest$$anonfun$1.apply(SimpleDateParamTest.scala:23)
    at org.scalatest.Transformer$$anonfun$apply$1.apply$mcV$sp(Transformer.scala:22)
    at org.scalatest.OutcomeOf$class.outcomeOf(OutcomeOf.scala:85)
    at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
    at org.scalatest.Transformer.apply(Transformer.scala:22)
    at org.scalatest.Transformer.apply(Transformer.scala:20)
    at org.scalatest.FunSuiteLike$$anon$1.apply(FunSuiteLike.scala:166)
    at org.scalatest.Suite$class.withFixture(Suite.scala:

Set timezone to fix SimpleDateParamTest

[srowen]

Nit: why import these so locally?
I'm slightly afraid this will have negative effects on other tests since you're setting the defaults globally for the JVM.
Why would this fail though if the date parameter specifies its time zone? it should not be ambiguous.

[scwf]

I'm slightly afraid this will have negative effects on other tests since you're setting the defaults globally for the JVM.

seems in spark all the date related tests use America/Los_Angeles time zone, so i think it will be ok.

Why would this fail though if the date parameter specifies its time zone? it should not be ambiguous.

I also do not get the root cause. but actually it is time zone sensitive, we can have a simple test for this, after we specify the timezone other than "America/Los_Angeles", this test will fail:

// Timezone is fixed to America/Los_Angeles for those timezone sensitive tests (timestamp_*)
      TimeZone.setDefault(TimeZone.getTimeZone("asia/shanghai"))
      // Add Locale setting
      Locale.setDefault(Locale.US)
     new SimpleDateParam("2015-02-20T23:21:17.190GMT").timestamp should be (1424474477190L)
     new SimpleDateParam("2015-02-20T17:21:17.190CST").timestamp should be (1424474477190L)
     new SimpleDateParam("2015-02-20").timestamp should be (1424390400000L)  // GMT

[srowen]

If so, then the whole test framework should set a default timezone globally. However, it's really not good to rely on this.

There seems to be a bug as this logic really can't depend on the current machine's timezone. "2015-02-20T23:21:17.190GMT" unambiguously names a point in time. So I don't htink we can commit this as a fix but rather root out the bug.

[scwf]

@srowen i think i get it. see https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/status/api/v1/SimpleDateParam.scala#L51
SimpleDateParam use SimpleDateFormat to parse the date string. SimpleDateFormat is sensitive for time zone and it use the default timezone if not set, we can set the time zone for simpledateformat by
SimpleDateFormat.setTimeZone

http://docs.oracle.com/javase/7/docs/api/java/text/SimpleDateFormat.html

[SparkQA]

Test build #33405 has finished for PR 6377 at commit 0588fba.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • "public class " + className + extendsText + " implements java.io.Serializable

[SparkQA]

Test build #33408 has finished for PR 6377 at commit 34bca75.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • "public class " + className + extendsText + " implements java.io.Serializable

[scwf]

/cc @squito

[squito]

Hi @scwf thanks for finding this. So I dug into this a little bit -- looks like this is just a little bit of ignorance on my part when creating the test. I used "CST" to mean "Central Standard Time", eg., the timezone of Chicago, because that's where I am. But if you're in Shanghai, "CST" will mean "China Standard Time". The code is already time zone sensitive, the problem is just ambiguity in the test.

I would like to have a test for some non-GMT timezone. How about we change it to something non-ambiguous? If we can trust http://www.timeanddate.com/time/zones/ we could use EST, which would make the equivalent time "2015-02-20T18:21:17.190EST".

Btw, another mistake a made: for consistency, the test class really should be SimpleDateParamSuite. Would you mind fixing that as well?

[scwf]

@squito updated, tested it and passed on my local machine.

[SparkQA]

Test build #33460 has finished for PR 6377 at commit 8bb74f0.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleDateParamSuite extends FunSuite with Matchers

[squito]

do you mind leaving the test case that when there is no timezone specified, GMT is assumed? I figure that if there is going to be some default, thats the only thing that makes sense. Or does the test not pass that way for you?

if you think there should not be any default, than we should change the behavior in SimpleDateParam (though I'd really prefer to leave it).

[SparkQA]

Test build #33485 has finished for PR 6377 at commit b8df1e5.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class SimpleDateParamSuite extends FunSuite with Matchers

[srowen]

This is a correct test though I think @squito's intent was to test the same point in time, 2015-02-20T18:21:17.190EST, as it did before. Anyway, already LGTM

[squito]

yeah, it doesn't really matter. this looks good, thanks for the fix! I'm merging this now

[scwf]

@squito https://github.com/apache/spark/blob/master/core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala#L85 also failed due to the time zone issue.
I set the default time zone fix this problem, but maybe it is not a good fix, Any idea to fix this?

sbt.ForkMain$ForkError: assertion failed: Expected:
[ {
  "id" : "local-1422981759269",
  "name" : "Spark shell",
  "attempts" : [ {
    "startTime" : "2015-02-03T16:42:38.277GMT",
    "endTime" : "2015-02-03T16:42:46.912GMT",
    "sparkUser" : "irashid",
    "completed" : true
  } ]
} ]
Found:
[ ]
Added:
[ {
  "id" : "local-1422981759269",
  "name" : "Spark shell",
  "attempts" : [ {
    "startTime" : "2015-02-03T16:42:38.277GMT",
    "endTime" : "2015-02-03T16:42:46.912GMT",
    "sparkUser" : "irashid",
    "completed" : true
  } ]
} ]

[squito]

can you change that line to the equivalent in GMT: applications?maxDate=2015-02-03T16:42:40.000GMT?
sigh, I never realized the problems CST would cause :P
sorry about this, thanks for tracking it down @scwf

[scwf]

it works, thanks @squito i will file a PR