search

aantron / bisect_ppx

Code coverage for OCaml and ReScript
http://aantron.github.io/bisect_ppx/demo/
MIT License
302 stars 60 forks source link

Runtime: --bisect-sigterm argument (#384) #390

Closed arvidj closed 2 years ago

arvidj commented 2 years ago

Here's a first stab at implementing the --bisect-sigterm option as discussed in (#384).

arvidj commented 2 years ago

Not sure what to do about the rescript runtime. I can probably attempt to the same thing for node if you think it is pertinent. Or, we could ignore this --bisect-sigterm in rescript (optionally fail when it is set).

aantron commented 2 years ago

I think it's best to ignore this in the ReScript runtime for now. Also, don't worry about errors in that — I can adapt it later.

aantron commented 2 years ago

The cram test looks good, thanks!

It looks like this PR has only the two remaining issues about when exactly SIGTERM is blocked and whether the handler is installed repeatedly, which should only need pretty small alterations. After that, is this code working in your test environment?

The ReScript test failures and the difference in spacing of wc output on Mac can be ignored. I can fix that later.

arvidj commented 2 years ago

The cram test looks good, thanks!

It looks like this PR has only the two remaining issues about when exactly SIGTERM is blocked and whether the handler is installed repeatedly, which should only need pretty small alterations. After that, is this code working in your test environment?

The first version worked in our environment, I'm gonna start testing now with the latest version :)

Do note though that I'm only testing this option in a smaller one of our tests, so that the fact that it works well in our environment is not necessarily a sign that works "everywhere". I'll get back when I have the test results.

arvidj commented 2 years ago

Yes, latest version seems to be working fine in our test suite.

arvidj commented 2 years ago

squashed

aantron commented 2 years ago

Thank you! I will do a 2.7.0 release soon.

arvidj commented 2 years ago

Thank you! I will do a 2.7.0 release soon.

Cool, thanks!

squashed

i just meant that i rewrote the history into one commit. Do you prefer to keep it in smaller commits? (for future mrs ;))

aantron commented 2 years ago

i just meant that i rewrote the history into one commit. Do you prefer to keep it in smaller commits? (for future mrs ;))

It's not necessary to squash (at least on GitHub), since GitHub has a merge-by-squashing button (which I use :P). On the other hand, squashing loses the continuity of review, i.e. during review the reviewer builds up an understanding up to the last commit in the PR. If the PR is then squashed (without agreement), the new commit is potentially an all-new diff and it can take work to make sure that the diff in that new commit is the same as the net diff the reviewer has already reviewed. So I usually ask not to force push into PRs after review has begun, except by agreement (it's fine to force push to replace a commit immediately after first pushing it, to quickly fix some errors if needed).

The vast majority of PRs are well addressed by GitHub's built-in squash-merge, turning them into one commit regardless of how messy the PR branch history was, and without the need for rewriting that history inside the PR (and threatening review). A few make more sense to merge as several commits, but then the maintainer can either do that themselves, or discuss with the PR author.

arvidj commented 2 years ago

i just meant that i rewrote the history into one commit. Do you prefer to keep it in smaller commits? (for future mrs ;))

It's not necessary to squash (at least on GitHub), since GitHub has a merge-by-squashing button (which I use :P). ...

Ah ok, I see. Sorry about that, I'm not so used to github. On the tezos/tezos repo we usually have authors squash their MRs into a single commit (for smaller stuff) or rewrite their history into a clean set of atomic commits (e.g. no fixups). Noted for future contributions!

aantron commented 2 years ago

Yep, this was also the practice on GitHub in the past (before squash-merge, when GitHub also seemed to be philisophically completely against squashing by anyone ever, didn't allow pushing into contributor branches, and it had to be negotiated :P). And in the case of this PR the squash didn't cause any problems, since I already had the branch locally for testing, so I was able to trivially do a diff to double-check all is well :)

Thanks again!

aantron commented 2 years ago

FYI I've changed BISECT_SIGTERM to use values yes/no rather than true/false, for consistency with Bisect's existing environment variables BISECT_ENABLE and BISECT_SILENT.

aantron commented 2 years ago

This and other recent changes are now in opam in Bisect_ppx 2.7.0.

arvidj commented 2 years ago

Great, thanks a lot Anton!