Atexit refactor with tests #3252

craigcomstock · 2018-09-07T19:37:46Z

See if combining the refactoring and tests to cover refactoring works well with coverage/travis/jenkins. This is the "right" way to do things.

merge together:
#3252
https://github.com/cfengine/enterprise/pull/429

cf-bottom · 2018-09-07T19:39:37Z

Thanks for submitting a PR! Maybe @vpodzime can review this?

codecov · 2018-09-07T20:32:02Z

Codecov Report

Merging #3252 into master will decrease coverage by 0.88%.
The diff coverage is 40.94%.

@@            Coverage Diff             @@
##           master    #3252      +/-   ##
==========================================
- Coverage   64.22%   63.34%   -0.89%     
==========================================
  Files         210      211       +1     
  Lines       46544    46593      +49     
==========================================
- Hits        29893    29514     -379     
- Misses      16651    17079     +428

Impacted Files	Coverage Δ
libutils/misc_lib.c	`37.5% <ø> (ø)`	⬆️
cf-agent/verify_exec.c	`73.98% <ø> (+2.31%)`	⬆️
cf-serverd/cf-serverd.c	`66.66% <0%> (ø)`
cf-serverd/cf-serverd-functions.c	`6.01% <0%> (+6.01%)`	⬆️
libpromises/parser.c	`83.52% <0%> (ø)`	⬆️
libpromises/cf3parse.y	`78.02% <0%> (+0.16%)`	⬆️
libutils/alloc.c	`88.88% <0%> (ø)`	⬆️
libutils/string_lib.c	`68.42% <0%> (+1.87%)`	⬆️
libpromises/fncall.c	`84.17% <0%> (ø)`	⬆️
libpromises/unix.c	`26.5% <0%> (ø)`	⬆️
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7eb0be7...64679e9. Read the comment docs.

craigcomstock · 2018-09-13T13:13:02Z

@olehermanse @vpodzime this PR is ready for review. I will squash that last "cleanup" commit. Do you think I should squash the two other commits as well? The first, 55565f9 is the refactor, fa2dcc9 is the tests.

olehermanse · 2018-09-13T13:20:03Z

I think the best way to look at it is:

Does the commit make more sense if you squash it or less?
Is it a standalone change that makes sense on it's own?
(If you were reviewing it, what git history would you like to read?)

craigcomstock · 2018-09-13T13:24:53Z

Right. I should have two commits: one refactor+test, one whitespace. Will do. :)

craigcomstock · 2018-09-13T14:28:40Z

Ok. Commits cleaned up. Thanks @olehermanse :)

craigcomstock · 2018-09-13T20:03:42Z

@olehermanse @vpodzime this is ready for a final code review if you would be so kind. When you are working of course. :) Before rewriting commit history all non-exotics passed on jenkins.

craigcomstock · 2018-09-20T12:47:00Z

@cf-bottom jenkins build thanks

cf-bottom · 2018-09-20T12:49:27Z

Sure, I triggered a build:

https://ci.cfengine.com/job/pr-pipeline/1186/

vpodzime

Looks good to me otherwise. The gdb integration is a really cool idea, we should extend it to increase coverage in other places too.

vpodzime · 2018-09-20T13:01:24Z

cf-agent/cf-agent.c

Weird indentation here.

Also, I think this is a debugging leftover, right?

vpodzime · 2018-09-20T13:06:13Z

libutils/logging.c

What happened to fsync(stdout)?

tests/acceptance/15_control/02_agent/simple_options.cf

tests/acceptance/16_cf-serverd/add_access_constraints_fail.cf

tests/acceptance/25_cf-execd/cli_options.cf

craigcomstock · 2018-09-24T20:55:19Z

@cf-bottom jenkins build please

cf-bottom · 2018-09-24T20:59:51Z

Alright, I triggered a build:

https://ci.cfengine.com/job/pr-pipeline/1200/

olehermanse · 2018-09-24T21:07:54Z

This pull request introduces 1 alert when merging 0ed576ccd957ccdf8b1b7da83e0cc5401dd5cd33 into eb7750c - view on LGTM.com

new alerts:

1 for Comparison result is always the same

Comment posted by LGTM.com

craigcomstock · 2018-09-24T21:33:52Z

@cf-bottom jenkins again

cf-bottom · 2018-09-24T21:34:42Z

Alright, I triggered a build:

https://ci.cfengine.com/job/pr-pipeline/1201/

craigcomstock · 2018-09-24T21:40:43Z

needed a rebase from upstream master. @cf-bottom another jenkins please

cf-bottom · 2018-09-24T21:44:34Z

Sure, I triggered a build:

https://ci.cfengine.com/job/pr-pipeline/1202/

craigcomstock · 2018-09-24T21:56:28Z

@cf-bottom oops, forgot that enterprise PR was included, adjusted there so jenkins build please

cf-bottom · 2018-09-24T21:59:31Z

Sure, I triggered a build:

https://ci.cfengine.com/job/pr-pipeline/1203/

vpodzime · 2018-09-25T12:44:33Z

Travis seems be consistently failing on this. 😞

craigcomstock · 2018-10-02T16:46:09Z

@olehermanse @vpodzime how would you guys feel about disabling these gdb driven tests on exotics and/or essentially if gdb version is less-than some value that works? Chasing down odd behavior in older versions seems a bit too much work and we already get a lot of benefit from these tests on many platforms.

craigcomstock · 2018-10-02T19:57:32Z

@cf-bottom if you would please, jenkins exotics

cf-bottom · 2018-10-02T19:59:44Z

Alright, I triggered a build:

(with exotics)

https://ci.cfengine.com/job/pr-pipeline/1284/

craigcomstock · 2018-10-03T14:39:16Z

@cf-bottom jenkins with exotics please

cf-bottom · 2018-10-03T15:14:26Z

Sure, I triggered a build:

(with exotics)

https://ci.cfengine.com/job/pr-pipeline/1290/

craigcomstock · 2018-10-03T16:29:36Z

@cf-bottom try again please: jenkins exotics

cf-bottom · 2018-10-03T16:30:20Z

Alright, I triggered a build:

(with exotics)

https://ci.cfengine.com/job/pr-pipeline/1291/

nickanderson

I found minor issues with test metadata. I did not do a deep dive, there are many test addtions here! +++

nickanderson · 2018-10-03T18:05:50Z

tests/acceptance/16_cf-serverd/add_access_constraints_fail.cf

This needs a meta attribute attached to it referencing the jira issue.
I think it may not skip properly without the issue reference.

"test_skip_needs_work" string => "redhat_4|debian_4|aix|debian_6|hpux", meta => { "ENT-3765" };

Also if you add a "description" string then the tests print that out when running.

You can see an example of failing output from tests that contain description and soft_fail here: https://tracker.mender.io/browse/CFE-2910

Thanks Nick. I'll take a look tomorrow. :)

nickanderson · 2018-10-03T18:07:41Z

tests/acceptance/16_cf-serverd/keep_promises_fail.cf

Same as other comment about meta attribute for jira issue.

nickanderson · 2018-10-03T18:08:33Z

tests/acceptance/27_cf-agent/couldnt_ioctl_socket.cf

meta attribute for jira issue

nickanderson · 2018-10-03T18:08:59Z

tests/acceptance/27_cf-agent/couldnt_open_socket.cf

meta attribute for jira issue

craigcomstock · 2018-10-03T21:51:45Z

@cf-bottom jenkins exotics please, now that I have rebased enterprise properly :p

cf-bottom · 2018-10-03T21:54:19Z

Sure, I triggered a build:

(with exotics)

https://ci.cfengine.com/job/pr-pipeline/1292/

craigcomstock · 2018-10-04T15:16:10Z

@cf-bottom another jenkins exotics try please

cf-bottom · 2018-10-04T15:20:18Z

Alright, I triggered a build:

(with exotics)

https://ci.cfengine.com/job/pr-pipeline/1294/

nickanderson

I see you added the meta tag to the meta promises that indicate the tests should be skipped on platforms where they need work.

LGTM.

resolved, not sure why github isn't auto-resolving it.

craigcomstock · 2018-10-08T20:31:45Z

@cf-bottom jenkins exotics please

cf-bottom · 2018-10-08T20:35:26Z

Alright, I triggered a build:

(with exotics)

https://ci.cfengine.com/job/pr-pipeline/1319/

craigcomstock · 2018-10-08T20:50:33Z

Well @cf-bottom, could you have mentioned that my dependent repo PR wasn't up-to-date with master? Maybe... oh well. Another try at jenkins + exotics please.

cf-bottom · 2018-10-08T20:55:05Z

Alright, I triggered a build:

(with exotics)

https://ci.cfengine.com/job/pr-pipeline/1320/

Jenkins is now green.

craigcomstock · 2018-10-09T02:01:46Z

I could probably improve coverage for cf-promises and cf-serverd but not by much and not for anything truly significant. I think this PR is ready to merge. Thanks for all the feedback/help with this monster.

Refactored use of atexit() to use of a custom cleanup mechanism. On Windows atexit() functions are called during the unloading process and we have some atexit() functions which rely on DLLs so this will not work reliably. Refactored so all platforms use RegisterCleanupFunction() and DoCleanupAndExit() instead of atexit() and exit().

craigcomstock · 2018-10-09T02:06:07Z

@cf-bottom jenkins exotics please -craig(ghi)

cf-bottom · 2018-10-09T02:10:15Z

Sure, I triggered a build:

(with exotics)

https://ci.cfengine.com/job/pr-pipeline/1322/

…ith-tests Atexit refactor with tests

This was referenced Sep 7, 2018

ENT-3999: Atexit code coverage #3197

Closed

ENT-3756: cf-agent intermittently hangs on windows #3180

Closed

craigcomstock force-pushed the atexit-refactor-with-tests branch from 3f2c639 to fa2dcc9 Compare September 12, 2018 20:04

olehermanse requested review from olehermanse and vpodzime September 13, 2018 13:17

craigcomstock force-pushed the atexit-refactor-with-tests branch from e23f747 to c3c8292 Compare September 13, 2018 14:28

craigcomstock force-pushed the atexit-refactor-with-tests branch from c3c8292 to 04c1afe Compare September 20, 2018 12:46

vpodzime reviewed Sep 20, 2018

View reviewed changes

craigcomstock force-pushed the atexit-refactor-with-tests branch from 90e88d6 to 0ed576c Compare September 24, 2018 20:54

craigcomstock force-pushed the atexit-refactor-with-tests branch from 0ed576c to 76195c0 Compare September 24, 2018 21:33

craigcomstock force-pushed the atexit-refactor-with-tests branch from 76195c0 to 0edf865 Compare September 24, 2018 21:40

craigcomstock force-pushed the atexit-refactor-with-tests branch from ae5c47e to a78d1de Compare October 3, 2018 14:02

nickanderson previously requested changes Oct 3, 2018

View reviewed changes

nickanderson approved these changes Oct 8, 2018

View reviewed changes

craigcomstock added 2 commits October 8, 2018 21:02

whitespace

64679e9

craigcomstock force-pushed the atexit-refactor-with-tests branch from 5b07fb5 to 64679e9 Compare October 9, 2018 02:02

craigcomstock merged commit 93428bf into cfengine:master Oct 9, 2018

olehermanse mentioned this pull request Oct 11, 2018

Revert "Atexit refactor with tests" #3325

Merged

craigcomstock added a commit to craigcomstock/core that referenced this pull request Jul 24, 2024

Merge pull request cfengine#3252 from craigcomstock/atexit-refactor-w…

526468c

…ith-tests Atexit refactor with tests

Atexit refactor with tests #3252

Atexit refactor with tests #3252

Uh oh!

Conversation

craigcomstock commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cf-bottom commented Sep 7, 2018

Uh oh!

codecov bot commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

craigcomstock commented Sep 13, 2018

Uh oh!

olehermanse commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craigcomstock commented Sep 13, 2018

Uh oh!

craigcomstock commented Sep 13, 2018

Uh oh!

craigcomstock commented Sep 13, 2018

Uh oh!

craigcomstock commented Sep 20, 2018

Uh oh!

cf-bottom commented Sep 20, 2018

Uh oh!

vpodzime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

craigcomstock commented Sep 24, 2018

Uh oh!

cf-bottom commented Sep 24, 2018

Uh oh!

olehermanse commented Sep 24, 2018

Uh oh!

craigcomstock commented Sep 24, 2018

Uh oh!

cf-bottom commented Sep 24, 2018

Uh oh!

craigcomstock commented Sep 24, 2018

Uh oh!

cf-bottom commented Sep 24, 2018

Uh oh!

craigcomstock commented Sep 24, 2018

Uh oh!

cf-bottom commented Sep 24, 2018

Uh oh!

vpodzime commented Sep 25, 2018

Uh oh!

craigcomstock commented Oct 2, 2018

Uh oh!

craigcomstock commented Oct 2, 2018

Uh oh!

cf-bottom commented Oct 2, 2018

Uh oh!

craigcomstock commented Oct 3, 2018

Uh oh!

cf-bottom commented Oct 3, 2018

Uh oh!

craigcomstock commented Oct 3, 2018

Uh oh!

cf-bottom commented Oct 3, 2018

Uh oh!

nickanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

craigcomstock commented Sep 7, 2018 •

edited

Loading

codecov bot commented Sep 7, 2018 •

edited

Loading

olehermanse commented Sep 13, 2018 •

edited

Loading