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

Finish PlainStatusBackend #636

Merged
merged 11 commits into from
Sep 11, 2020

Conversation

ralismark
Copy link
Contributor

#487 implements most of this but appears abandoned. This fixes some of the comments left on those changes.

@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

Merging #636 into master will increase coverage by 0.31%.
The diff coverage is 28.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
+ Coverage   45.94%   46.25%   +0.31%     
==========================================
  Files         138      139       +1     
  Lines       60587    60448     -139     
==========================================
+ Hits        27834    27958     +124     
+ Misses      32753    32490     -263     
Impacted Files Coverage Δ
src/bin/tectonic.rs 0.00% <0.00%> (ø)
src/status/mod.rs 59.25% <0.00%> (+0.63%) ⬆️
src/status/plain.rs 0.00% <0.00%> (ø)
src/status/termcolor.rs 37.37% <0.00%> (-1.58%) ⬇️
src/driver.rs 60.50% <70.00%> (-1.70%) ⬇️
tests/executable.rs 89.79% <90.00%> (+66.91%) ⬆️
src/errors.rs 6.38% <0.00%> (+0.02%) ⬆️
... and 13 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 1a200dd...0dbaf2a. Read the comment docs.

@pkgw
Copy link
Collaborator

pkgw commented Sep 9, 2020

Thanks again for your contribution! I especially appreciate your working from @Mrmaxmeier's branch to as to preserve their commits in the history.

With your update my only comment is about the test coverage. It would be nice to add something there to keep the coverage moving in the right direction. I think if you add a case or two to test/executable.rs that use the new color=no option, that should do the trick?


#[test]
fn test_no_color() {
if env::var("RUNNING_COVERAGE").is_ok() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh ... I was confused by why you new tests didn't seem to have any coverage, but here it is disabled in the coverage run!

Looking in the history, it is not clear to me why these tests get disabled during the code-coverage tests. @Mrmaxmeier I think you added these tests originally ... do you remember why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vaguely remember something about the coverage tracking not working for end-to-end tests that spawn a new tectonic process. Not sure if that's still an issue though.

Some of these failed due to a missing environment variable, but if I fix
that the tests at least seem to run, in my local testing.
@pkgw
Copy link
Collaborator

pkgw commented Sep 10, 2020

I've pushed some commits that will try to re-enabled the executable tests in the coverage run. I'm not sure if they will correctly add to the coverage stats (as per @Mrmaxmeier's comment), but hopefully they'll at least run.

@pkgw
Copy link
Collaborator

pkgw commented Sep 10, 2020

OK I think it's the case that the executable tests are running, but the coverage induced by executing the tectonic program is not being recorded. I'll do a bit of research to if there's anything that we can do about that ... hopefully someone has tackled this problem before.

If that doesn't work out we can certainly go ahead and merge this, but I think it's worth some effort trying to solve this.

@pkgw
Copy link
Collaborator

pkgw commented Sep 10, 2020

xref: assert-rs/assert_cmd#9

@pkgw
Copy link
Collaborator

pkgw commented Sep 11, 2020

OK, I did a little experiment. If I hack up the executable test harness to manually launch its tectonic subprocesses inside kcov, I can make it so that we do gather coverage information from those invocations. Doing so gains us about 1.3% in code coverage (~1000 lines) compared to the current baseline. I think I could wire this up in the CI/CD framework, but it would be a bit hacky and ugly.

Alternatively, we could work on revising the executable-type tests to use Tectonic as a library, with an absolutely minimal amount of code in src/main.rs (or its moral equivalent).

Or, we could keep those tests as-is and just keep in mind that they won't contribute to the code coverage stats.

I am not sure which of these options I prefer. Any opinions?

@pkgw
Copy link
Collaborator

pkgw commented Sep 11, 2020

OK, well, I am still interested in any thoughts about this broader question, but work along those lines is best saved for a separate PR. So, let's merge this one!

@pkgw pkgw merged commit 64db8d7 into tectonic-typesetting:master Sep 11, 2020
pkgw added a commit to pkgw/tectonic that referenced this pull request Nov 1, 2020
This change in behavior was introduced in tectonic-typesetting#636 with the genericization
of the status backend. Add a new `report_error` method to recover the
old behavior.

Closes tectonic-typesetting#665.
@ralismark ralismark deleted the plain-status-backend branch April 14, 2021 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants