Skip to content

Conversation

@sm11963
Copy link
Contributor

@sm11963 sm11963 commented Nov 20, 2018

This outputs Coveralls JSON like so:

{
  "service_job_id": "1234567890",
  "service_name": "travis-ci",
  "source_files": [
    {
      "name": "example.ml",
      "source_digest": "asdfasdf1234asfasdf2345",
      "coverage": [null, 1, null]
    },
    {
      "name": "lib/two.ml",
      "source_digest": "asdf1234asdfsdfggsfgd9423",
      "coverage": [null, 1, 0, null]
    }
  ]
}

I have tested sending to Coveralls successfully and manually verified the results again the existing html report. I additionally modified tests to add a line which has both visited and unvisited points (line 29 in fixtures/report/source.ml) this is important to test since I define such a line to have coverage of 0 when generating a Coveralls JSON report. BTW let me know if there are any clever ways to get the test coverage back to 50%, it seemed intentional and is such a nice number 😄 .

Reasons to merge:

  • Ocveralls seems to be no longer maintained, when testing it did not correctly parse the bisect_ppx output files.
  • Ocveralls uses the original Bisect, and as such requires a GPLv3 license which is more restrictive.
  • This is a fairly lightweight report generator to maintain with no dependencies.
  • I see some code that may be able to be removed / cleaned up if we decide to no longer support Ocveralls.

Downsides:

  • Adds a very specific reporter to a list of what seems like generic reporters
  • Adds several argument flags to the reporter command
  • I did not add the capability to send to Coveralls as part of the reporter command, that seems out of scope of this tool.

@sm11963 sm11963 force-pushed the sm11963/addCoveralls branch 3 times, most recently from aff335e to 9edf12e Compare November 21, 2018 01:40
@aantron
Copy link
Owner

aantron commented Nov 21, 2018

Thanks!

I will review shortly (perhaps tomorrow or Thursday).

However, on the meta: perhaps ocveralls is maintained on demand? I don't see that the repo has e.g. issues that have gone unanswered, it looks more like people aren't actively working on projects that use ocveralls, so nobody has noticed that bit rot has set in. But if you were to try contributing to it, perhaps you'd get a speedy response. cc @sagotch.

Given that this PR doesn't send the report, we need either instructions on how to do that, or to keep relying on ocveralls.

I see the issue about GPLv3, but perhaps we can persuade @sagotch to drop support for the original Bisect in the next release of ocveralls, and change the license to MIT, BSD, etc.

@sagotch
Copy link

sagotch commented Nov 21, 2018

No problem about dropping support of the original Bisect in a next release, and I expose most of my work with the MIT license if not forced by others to use something else.

I do not use ocveralls in any of my active projects, and that is why it is currently unmaintain, and I have no idea about what is currently working or not... But I'll be glad to draft a new release if you want to contribute to this project.

I do not have a strong opinion about it but it seems that the whole coveralls thing is out of scope of this tool. So I would say that an external package like ocveralls a better-suited solution to this.

If you think that this is easier and cleaner to have support for coveralls directly in bisect_ppx, I would have no problem at all about deprecating my thing in favor of this solution.

@aantron
Copy link
Owner

aantron commented Nov 21, 2018

@sagotch, if you don't see yourself becoming a user of ocveralls again in the near future, you may consider either adding several more people to the repo (perhaps @sm11963), or else transferring it to ocaml-community.

My first instinct is that full coveralls support requires a separate tool, because it needs a way to upload reports. I don't know what is the absolute easiest or most natural way to do that, but ocveralls was pretty easy to use, while I suspect the dependency load from essentially integrating ocveralls into Bisect_ppx will be too much for most of the Bisect_ppx users that don't use coveralls.

@sagotch
Copy link

sagotch commented Nov 21, 2018

A first cleanup and a new release would probably a good thing to do before transferring it to ocaml-community.

@sm11963, do you think that you could make a PR to ocveralls in order to fix what is fixed here so I could release a new version of ocveralls and transfer something in a good state to ocaml-community?

@sm11963
Copy link
Contributor Author

sm11963 commented Nov 21, 2018

A first cleanup and a new release would probably a good thing to do before transferring it to ocaml-community.

@sm11963, do you think that you could make a PR to ocveralls in order to fix what is fixed here so I could release a new version of ocveralls and transfer something in a good state to ocaml-community?

I can take a look to see what I can do, but I'd like to repeat my main reasons for putting up a PR here rather than ocveralls:

  1. I am unable to use ocveralls in the project I would really like to use this in (uber/NEAL) due to the GPLv3 license restrictions
  2. Ocveralls has not been updated in 2 years so I was uncertain of its state
  3. Ocveralls doesn't seem to provide the functionality that -I provides to include multiple search directories for source files, which would yet be another thing that I would need to add to ocveralls for my use case.
  4. I feel that since bisect_ppx does provide a suite of reporters, so it makes sense to me to be able to output a Coveralls report from this tool. With the frameworks already in place it seems like its easier to add different reporters here then implementing them in an external project. If bisect-ppx-report was moved to an external library or another sort of plugin method it would make sense to me to keep them separate.

@sm11963
Copy link
Contributor Author

sm11963 commented Nov 21, 2018

Given that this PR doesn't send the report, we need either instructions on how to do that, or to keep relying on ocveralls.

I actually added instructions in the readme already. Sending to Coveralls it is a trivial curl command, so I don't feel its critical to add that functionality into the tool that generates the json.

@aantron
Copy link
Owner

aantron commented Nov 21, 2018

Ah, thanks! I must have scrolled past the README in a hurry to get to the code.

@sagotch
Copy link

sagotch commented Nov 21, 2018

I let @aantron choosing where this should go.

  1. As I said, license is not a problem because I would remove original bisect support and change license to MIT or whatever you want.

  2. Probably not used once in 2 years...

  3. -I option seems quite trivial to add.

  4. Good point, I did not know about the "framework"

@sm11963 sm11963 force-pushed the sm11963/addCoveralls branch from 9edf12e to 98472ad Compare November 21, 2018 21:12
Copy link
Owner

@aantron aantron left a comment

Choose a reason for hiding this comment

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

I think we can merge this into Bisect. I have just the one question about the README.

README.md Outdated
-I _build/ \
-coveralls coverage.json \
-service-name travis-ci \
-service-job-id 1234567 \
Copy link
Owner

Choose a reason for hiding this comment

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

If using Travis as the example, should this perhaps be $TRAVIS_JOB_ID or another environment variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can make a new doc page to cover the environment variables / other usages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated to reference the variable for now, which may be sufficient. It may be nice however to document the environment variables that ocveralls supported:
https://github.com/sagotch/ocveralls/blob/master/src/ocverallsCI.ml#L26-L30

@sagotch
Copy link

sagotch commented Nov 23, 2018

I will be deprecating ocveralls in favor of the built-in backend.

You should ask coveralls to update this https://docs.coveralls.io/ocaml (I don't know how).

README.md Outdated

### Coveralls.io

You can submit a coverage report to Coveralls.io using [ocveralls][ocveralls] or
Copy link

Choose a reason for hiding this comment

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

You should remove this one :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done

@sm11963 sm11963 force-pushed the sm11963/addCoveralls branch from 1470dc6 to 566ae8b Compare November 23, 2018 17:55
@sm11963 sm11963 force-pushed the sm11963/addCoveralls branch from 566ae8b to 86bd446 Compare November 23, 2018 17:59
@aantron
Copy link
Owner

aantron commented Nov 24, 2018

@sm11963 Thanks!

@sagotch

I will be deprecating ocveralls

It's been a pleasure working with you over the years, and I was a happy user of ocveralls :)

I'll be sure to ping the Coveralls people once there is a Bisect release.

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