search

coverallsapp / coverage-reporter

Self-contained, universal coverage uploader binary. Under development.
https://coveralls.io
MIT License
45 stars 14 forks source link

Reinstate updated python parser #122

Closed littleforest closed 2 months ago

littleforest commented 2 months ago

:zap: Summary

This reinstates the work done in #116 that was rolled back in #120. We found that users sometimes had a .coverage file, but did not have the coverage program installed, causing an error to be raised.

This adds a check for the coverage executable in the matches? method for the python parser. If a file matches the python format, then we also do a secondary check to make sure that coverage is installed. If not, then we output a message about the missing program when the debug flag is passed.

coveralls-official[bot] commented 2 months ago

Pull Request Test Coverage Report for Build 8725545615

Details

Totals Coverage Status
Change from base Build 8638441637: 0.2%
Covered Lines: 908
Relevant Lines: 967
💛 - Coveralls
littleforest commented 2 months ago

Can we / shouldn't we also deliver the error even when --debug flag is not used? Otherwise, how will the user know why the upload failed?

We can deliver it all the time. However, the upload might not have failed in the case that the user is not specifying a format. This was trying to cover the situation where the user has a coverage.xml file (for example), but also a .coverage file. The coverage.xml file goes through the Cobertura parser just fine, but then the .coverage file gets hung up because the user didn't have coverage installed. The changes here are such that we don't consider the .coverage file to "match" the requirements for the python parser if coverage is not installed, and we just note that in the output message.

afinetooth commented 2 months ago

I left some additional comments in our Slack thread on this, here.

We can deliver it all the time. However, the upload might not have failed in the case that the user is not specifying a format. This was trying to cover the situation where the user has a coverage.xml file (for example), but also a .coverage file. The coverage.xml file goes through the Cobertura parser just fine, but then the .coverage file gets hung up because the user didn't have coverage installed.

OK, got it. In that case, I would say the determining factor as to whether to deliver an error is:

  1. If the user specified format: cobertura - Don't deliver an error. The integration had what it needed to parse and upload a coverage report.
  2. If the user specified format: python - Deliver an error in both --debug and non---debug states. The integration cannot meet the requirement of uploading a .coverage report (without coverage being present).
  3. If the user did not specify format (format: null) - Only deliver an "error" / "warning" message in --debug state. We don't need to deliver the error message outside of --debug state because the integration can meet the requirement of uploading some coverage report. but it would be helpful to let the user know that there was a coverage report (in python format) that the integration found but could not upload (via its python parser).

WDYT?

The changes here are such that we don't consider the .coverage file to "match" the requirements for the python parser if coverage is not installed, and we just note that in the output message.

Agreed.

I just think that in each of the states above it the message a different level of import. In case (2) it's an error. In case (3) it's a warning, because at least one coverage report was uploaded.

littleforest commented 2 months ago

@afinetooth here are the following updates

  1. If the user specified format: cobertura - Don't deliver an error. The integration had what it needed to parse and upload a coverage report.

This will not hit the python parser at all (existing behavior), so no warning or error will be displayed.

  1. If the user specified format: python - Deliver an error in both --debug and non---debug states. The integration cannot meet the requirement of uploading a .coverage report (without coverage being present).

When a format is specified, the code does not use the matches? method so the check in the matches? method to see if coverage is installed will be bypassed. Instead, the code will attempt to use the coverage executable on the file directly. Currently this will raise an error if coverage is not installed, but me know if you would prefer to use Log.error (see 20f872bc2442926c1ec82cafe5aef39a7e4cf196 for the updated copy output).

  1. If the user did not specify format (format: null) - Only deliver an "error" / "warning" message in --debug state.

This will deliver a warning in --debug state: 

☝️ Detected coverage format: python, but error with coverage executable: sh: coverage: command not found
afinetooth commented 2 months ago

@littleforest this looks good, thank you! I think these choices are fine and reasonable. If feedback differs we can iterate from here. Thanks again.