Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
icinga-migration opened this issue Feb 11, 2016 · 15 comments

Comments

Projects
None yet
2 participants
@icinga-migration
Copy link
Member

commented Feb 11, 2016

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

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:

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

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

  • Relates set to 11147
@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

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

This comment has been minimized.

Copy link
Member Author

commented May 11, 2016

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

This comment has been minimized.

Copy link
Member Author

commented May 11, 2016

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

This comment has been minimized.

Copy link
Member Author

commented May 11, 2016

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

This comment has been minimized.

Copy link
Member Author

commented May 11, 2016

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

This comment has been minimized.

Copy link
Member Author

commented May 11, 2016

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

This comment has been minimized.

Copy link
Member Author

commented May 11, 2016

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

This comment has been minimized.

Copy link
Member Author

commented May 11, 2016

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

This comment has been minimized.

Copy link
Member Author

commented May 20, 2016

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

This comment has been minimized.

Copy link
Member

commented Jan 19, 2018

Fixed #5871

@Crunsher Crunsher closed this Jan 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.