-
Notifications
You must be signed in to change notification settings - Fork 180
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
cobertura task #302
cobertura task #302
Conversation
fe0d7ab
to
5503116
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR. Could you let me ask for some clarification?
lib/excoveralls/cobertura.ex
Outdated
|
||
timestamp = DateTime.utc_now() |> DateTime.to_unix(:millisecond) | ||
|
||
version = "1.9.4.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you help me understand what this version mean? (description or reference document)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if this version is meaningful for end users, it's worthwhile to note in the README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cobertura is the name of the java software that generates those coverage reports. The version on the XML file is the version of the tool. Since we are not using the tool, that value is not meaningful but in the DTD schema is marked as required so I put there the last version of cobertura I'm aware of.
The same is done in the erlang covertool package for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment.
Since we are not using the tool, that value is not meaningful but in the DTD schema is marked as required so I put there the last version of cobertura I'm aware of.
I am looking at the following page, but it shows some description around version 2 (ex. 2.1.1
), dated around 2015. Does it make sense to put 2.1.1
? or put in a less-meaningful values rather than putting the last version of 1.9.x
?
I understood that this version itself isn't meaningful, but just wanted to clarify the current meaning, as I am not so familiar with the current status of cobetura (though I once used the tool some time ago).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm not so familiar with this tool too. It makes sense to put the latest version. I would also put something like 0.0.0
to document that we are not using that field but then I don't know if we break some possible usage.
For the moment I'll change it to 2.1.1
Thanks for the PR 🙇 . I wasn't aware of this type of usage. I am having certain concerns around maintenance for adding new types, but it seems the format is stable (not updated recently). If it doesn't break other features, and if not-constant updates are required, it may be one option to include this subcommand. Do you have any comments / insights around the compatibility / stability for this feature support? (considering the maintenance). |
The cobertura tool seems very stable (if not freezed). The last commit (as you were noticing) is from 2015 and the last edit to the DTD schema was made in 2013. I don't expect any change to the excoveralls code that exports this report in the near future. |
Thanks for the additional comments:bow: |
A taks is added for generating XML Cobertura reports
This is useful for instance to enable the Test coverage visualization feature of GitLab.