aaOpenSource / aaLog

A library and example programs for reading the standard SMC log file format
MIT License
13 stars 15 forks source link

Unit tests fail due to time values #22

Closed logic-danderson closed 8 years ago

logic-danderson commented 8 years ago

A good number of the unit tests in aaLogReaderTests and aaLogReaderTests fail on my machine because the times are off by an hour. For example, aaLogReaderTests.ReadLogFileReadHeader expects a StartDateTime of 2015-10-23T14:53:01.5872834-04:00, but my machine comes up with 2015-10-23T13:53:01.5872834-05:00.

It looks like a time zone issue. I am in the Central time zone and it looks like the tests were written in the Eastern time zone.

I'm not sure what the best way to fix this would be. Is the problem in the tests or in the code they're testing? Do the log files indicate which time zone they are in?

arobinsongit commented 8 years ago

I've been wondering about this myself. As far as I can tell there is no additional metadata with the timezone. Maybe we could look at some common aaLGX and aaLog files and report back on our findings using the built-in log viewer from WW to see if it can tell the difference.

logic-danderson commented 8 years ago

The time zone must be in there somewhere. I opened the test.aaLGX file used by aaLgxReaderTests in System Management Console. The CanReadFirstRecord test expects to see 6:49:41 am and that's the time shown in my SMC. But the test fails expecting to get 7:49:41 am. So the SMC knows about the time zone difference.

arobinsongit commented 8 years ago

Yeah, definitely sounds like it knows something. I'll see if I can dig a little this weekend. Problem is that I believe I have all the header fields accounted for

ryanvs commented 8 years ago

I have the same problem and that is the difference between -04:00 (yours) and -05:00 (mine) time zones. There are several options, including:

  1. adding StartDateTimeUtc and EndDateTimeUtc methods and marking the local versions with [JsonIgnore]. This appears to work but you have to change all of the reference files.
  2. Possibly changing StartDateTime and EndDateTime from DateTime to DateTimeOffset to contain time zone information
  3. Or just editing the test and manually removing the offending values, e.g. var expectedObj = JObject.Parse(refHeaderJSON); expectedObj.Remove("StartDateTime");. This has the advantage of not needing to edit all of the reference files, but you have to edit the tests instead. Of course you could put that in a simple method as well.

I tried suggestions (1) and (3) and I still had another test exception with HostFQDN. It appears the code uses the current computer for the FQDN value. I guess the log file does not contain the original Host FQDN?

logic-danderson commented 8 years ago

I think ideally the code would pull the time zone out of the log file and use it. Time zone information has to be in that file somewhere because System Management Console handles it. Header bytes 12 through 19 are not mapped out yet. I took a quick look at the values to see if they contain time zone info but didn't get anywhere. I'll look again.

If we can get that figured out, then I agree that the code should use DateTimeOffset to contain the time zone. If the logs are local to the program that reads them it probably wouldn't matter so much. But I'm looking at using this library for a company that has sites across different time zones, so it'll be important to have the correct times.

Also, I think the tests should be changed so they don't compare all of the object's properties at once against a hard coded string. Instead test each property individually. That would make it easier to accommodate the differences between our machines, including the host FQDN. And if we stick with assuming the date time values are local, then those would be easier to verify if they aren't compared to hard coded strings, too.

The computer name is in the header and it is populated in the ComputerName property. I see where it's also handy to have the name of the computer that read the log, which is what the host FQDN contains. But obviously that varies from host to host so it can't be hard coded in the tests.

Just my two cents.

logic-danderson commented 8 years ago

Well, never mind about determining the time zone. It turns out the unit tests have the expected results and actual results switched around. The test results say they expect 6:49:41 am and are getting 7:49:41 am instead, but that's backwards. I didn't notice that until now.

The unit tests on my machine read 6:49:41 am from the aaLGX file. And the SMC shows the same time. So SMC is not adjusting the time zone like I thought. Sorry, should have been more thorough.

The unit tests are still broken, though. I can change the tests so they do not use hard coded string comparisons.

And now I'm not sure how to handle that multiple site scenario I mentioned earlier. Hmmm...

arobinsongit commented 8 years ago

Can you check on this item and see if it is resolved so we can close this issue?

logic-danderson commented 8 years ago

All of the tests on the master branch pass, so I think it's good.