[dev.icinga.com #11146] Unit test for LegacyTimePeriod

[icinga-migration]

This issue has been migrated from Redmine: https://dev.icinga.com/issues/11146

Created by atj on 2016-02-11 15:06:51 +00:00

Assignee: atj
Status: Assigned (closed on 2016-05-11 07:26:41 +00:00)
Target Version: (none)
Last Update: 2016-05-20 10:17:08 +00:00 (in Redmine)

Backport?: Not yet backported
Include in Changelog: 1

---

Whilst fixing #11132 I wrote a unit test for LegacyTimePeriod, which I have attached. The patch includes some additional tests for the "day X" and "day -X" timespecs, which are currently failing:

Test project /home/ajames/Sources/icinga2/build
Constructing a list of tests
Done constructing a list of tests
Checking test dependency graph...
Checking test dependency graph end
test 58
    Start 58: base-icinga_legacytimeperiod/simple

58: Test command: /home/ajames/Sources/icinga2/build/Bin/Release/boosttest-test-base "--run_test=icinga_legacytimeperiod/simple" "--catch_system_error=yes"
58: Test timeout computed to be: 1500
58: Running 1 test case...
58: /home/ajames/Sources/icinga2/test/icinga-legacytimeperiod.cpp(82): error: in "icinga_legacytimeperiod/simple": check mktime(&beg) == (time_t) 1343689200 has failed [1343520000 != 1343689200]
58: /home/ajames/Sources/icinga2/test/icinga-legacytimeperiod.cpp(83): error: in "icinga_legacytimeperiod/simple": check mktime(&end) == (time_t) 1346367600 has failed [1343606400 != 1346367600]
58: /home/ajames/Sources/icinga2/test/icinga-legacytimeperiod.cpp(88): error: in "icinga_legacytimeperiod/simple": check mktime(&beg) == (time_t) 1456704000 has failed [1456531200 != 1456704000]
58: /home/ajames/Sources/icinga2/test/icinga-legacytimeperiod.cpp(89): error: in "icinga_legacytimeperiod/simple": check mktime(&end) == (time_t) 1456790400 has failed [1456617600 != 1456790400]
58: 
58: *** 4 failures are detected in the test module "icinga2_test"
1/1 Test #58: base-icinga_legacytimeperiod/simple ...***Failed    0.05 sec

0% tests passed, 1 tests failed out of 1

Total Test time (real) =   0.05 sec

The following tests FAILED:
     58 - base-icinga_legacytimeperiod/simple (Failed)
Errors while running CTest

The returned values are the second to last day of the month. A separate issue will be created to address this.

Please review the patch and confirm if it is acceptable. I will write some additional tests if so as it seems like there are some bugs lurking in the code.

Attachments

• legacytimeperiod-unit-test.patch atj - 2016-02-11 15:06:40 +00:00 - Version 1
• 0001-Add-unit-test-for-LegacyTimePeriod.patch atj - 2016-05-20 10:10:52 +00:00

Changesets

2016-05-11 07:23:39 +00:00 by atj 1246d7d

Implement unit tests for the time period parser

fixes #11146

2016-05-11 16:12:20 +00:00 by (unknown) fc889eb

Revert "Implement unit tests for the time period parser"

This reverts commit 1246d7dda334859ee136198b8285f5f4f9504b59.

refs #11146

---

Relations:

• relates #11146

[icinga-migration]

Updated by mfriedrich on 2016-03-18 13:25:38 +00:00

• Status changed from New to Assigned
• Assigned to set to jflach
• Target Version set to 2.5.0

[icinga-migration]

Updated by mfriedrich on 2016-03-18 13:25:57 +00:00

• Relates set to 11147

[icinga-migration]

Updated by jflach on 2016-03-18 14:48:15 +00:00

• Status changed from Assigned to Feedback
• Assigned to changed from jflach to atj

Looks good, I will merge this once Bug #11147 is fixed. You offered to submit a patch, does that offer still stand? :D

[icinga-migration]

Updated by atj on 2016-03-18 14:56:18 +00:00

jflach wrote:

Looks good, I will merge this once Bug #11147 is fixed. You offered to submit a patch, does that offer still stand? :D

Yes, I'm still happy to write a patch. Is there a preference on submitting diffs here vs. filing PRs on github?

[icinga-migration]

Updated by jflach on 2016-03-22 12:35:47 +00:00

Sorry for the slow response, I forgot to set myself as watcher.

We prefer git formatted patches uploaded here, github pr fit awkwardly into our workflow.

[icinga-migration]

Updated by mfriedrich on 2016-04-22 09:03:29 +00:00

You can go with PRs containing a single commit (rebase, squash) as well. Adding a ".patch" after the url for the PR allows you to do magic things like

curl https://github.com/Icinga/icinga2/pull/76.patch | git am -s

[icinga-migration]

Updated by gbeutner on 2016-05-11 07:18:44 +00:00

The timestamps seem to be off by an hour:

% clock format 1343689200
Mon Jul 30 23:00:00 UTC 2012
% clock format 1346367600
Thu Aug 30 23:00:00 UTC 2012

[icinga-migration]

Updated by gbeutner on 2016-05-11 07:23:27 +00:00

-       BOOST_CHECK_EQUAL(mktime(&beg), (time_t) 1343689200); // 2012-07-31
-       BOOST_CHECK_EQUAL(mktime(&end), (time_t) 1346367600); // 2012-08-01
+       BOOST_CHECK_EQUAL(mktime(&beg), (time_t) 1343692800); // 2012-07-31
+       BOOST_CHECK_EQUAL(mktime(&end), (time_t) 1343779200); // 2012-08-01

[icinga-migration]

Updated by gbeutner on 2016-05-11 07:26:36 +00:00

• Status changed from Feedback to Assigned
• Target Version changed from 2.5.0 to 2.4.8

[icinga-migration]

Updated by atj on 2016-05-11 07:26:41 +00:00

• Status changed from Assigned to Resolved
• Done % changed from 0 to 100

Applied in changeset 1246d7d.

[icinga-migration]

Updated by gbeutner on 2016-05-11 16:07:48 +00:00

This breaks the Windows build:

6>  icinga-legacytimeperiod.cpp
6>C:\Users\Gunnar\Documents\Visual Studio 2015\Projects\icinga2\test\icinga-legacytimeperiod.cpp(34): error C3861: 'setenv': identifier not found
6>C:\Users\Gunnar\Documents\Visual Studio 2015\Projects\icinga2\test\icinga-legacytimeperiod.cpp(41): error C3861: 'setenv': identifier not found
6>C:\Users\Gunnar\Documents\Visual Studio 2015\Projects\icinga2\test\icinga-legacytimeperiod.cpp(43): error C3861: 'unsetenv': identifier not found
6>C:\Users\Gunnar\Documents\Visual Studio 2015\Projects\icinga2\test\icinga-legacytimeperiod.cpp(53): error C3861: 'timegm': identifier not found

[icinga-migration]

Updated by gbeutner on 2016-05-11 16:07:59 +00:00

• Status changed from Resolved to Assigned
• Assigned to changed from atj to gbeutner

[icinga-migration]

Updated by gbeutner on 2016-05-11 16:13:03 +00:00

• Assigned to changed from gbeutner to atj
• Target Version deleted 2.4.8

I have reverted this patch (1246d7d) for now. Please provide an updated patch that works on Windows.

[icinga-migration]

Updated by atj on 2016-05-20 10:17:08 +00:00

• File added 0001-Add-unit-test-for-LegacyTimePeriod.patch

Please try the updated version of the patch attached. Your commit included a call to timegm to fill the ref timestruct which was not in my original patch and is not included in this version as the tests pass without it. If it was added for a specific reason then please let me know.

The changes I have made for Windows compatibility are based purely on the Microsoft documentation as unfortunately I don't have access to a Window system with Visual Studio etc.

[Crunsher]

Fixed #5871