Finish PlainStatusBackend #636
Conversation
This avoids monomorphization and sipmlifies the API a bit.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
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 |
tests/executable.rs
Outdated
|
|
||
| #[test] | ||
| fn test_no_color() { | ||
| if env::var("RUNNING_COVERAGE").is_ok() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
I've pushed some commits that will try to re-enabled the |
|
OK I think it's the case that the executable tests are running, but the coverage induced by executing the 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. |
|
xref: assert-rs/assert_cmd#9 |
|
OK, I did a little experiment. If I hack up the Alternatively, we could work on revising the 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? |
|
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! |
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.
#487 implements most of this but appears abandoned. This fixes some of the comments left on those changes.