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

Add code coverage: tarpaulin / codecov #411

Merged
merged 17 commits into from
Jun 1, 2020
Merged

Add code coverage: tarpaulin / codecov #411

merged 17 commits into from
Jun 1, 2020

Conversation

Lan2u
Copy link

@Lan2u Lan2u commented May 22, 2020

Relates to issue #378.

Added tarpaulin action to the test suite on Linux section of the CI, the produced report is then submitted to codecov.io. I used my own personal token for testing the codecov part but have removed it for this commit (it can be re-added into the actions file - this is why that stage currently fails).

@Lan2u
Copy link
Author

Lan2u commented May 22, 2020

I haven't done much with this except get tarpaulin to run, this is my first attempt to work on an issue as part of an open-source project so if I have missed the mark please let me know.

@Razican Razican added the test Issues and PRs related to the tests. label May 22, 2020
@Razican Razican added this to the v0.9.0 milestone May 22, 2020
@Razican Razican linked an issue May 22, 2020 that may be closed by this pull request
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution!
This looks very good, check my comments, though.

It seems that you removed the Cargo.lock file. This is not a big deal, it can be regenerated by running cargo update.

We also don't need two workflows, so we don't need .github/workflows/coverage.yml files.

If this is your first contribution to open source, congrats! keep them coming! :)

.github/workflows/rust.yml Outdated Show resolved Hide resolved
.github/workflows/rust.yml Outdated Show resolved Hide resolved
.github/workflows/rust.yml Outdated Show resolved Hide resolved
Comment on lines 93 to 94
with:
token:
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to specify a token, since this is a public repository. This can be removed :)

Copy link
Author

Choose a reason for hiding this comment

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

I removed this but it seems to still fail? Perhaps it is because I'm trying from my fork of the repo.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is because we still don't have the project in CodeCov. Public repos shouldn't need a token.

Paul Lancaster and others added 6 commits May 22, 2020 22:46
@Lan2u Lan2u requested a review from Razican May 25, 2020 00:02
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

It seems you modified the Cargo.lock file. Could we avoid this? It's giving merge conflicts, but there should be no modification to that file :)

@Lan2u
Copy link
Author

Lan2u commented May 26, 2020

It seems you modified the Cargo.lock file. Could we avoid this? It's giving merge conflicts, but there should be no modification to that file :)

Done, sorry I thought I already removed it.

@Razican
Copy link
Member

Razican commented May 26, 2020

Ok, let's wait for @jasonwilliams to see if we can have Boa in codecov so that this works :)

@jasonwilliams
Copy link
Member

I've now added the repo, give it a try and let us know

@Lan2u
Copy link
Author

Lan2u commented May 31, 2020

I've now added the repo, give it a try and let us know

Tried again but still codecov fails saying it needs a token

@jasonwilliams
Copy link
Member

@Lan2u could you try again?

@Lan2u
Copy link
Author

Lan2u commented Jun 1, 2020

@Lan2u could you try again?

Tried again, the same result unfortunately :(

@Razican
Copy link
Member

Razican commented Jun 1, 2020

I think it just needs to be initialized. We could try to merge this without the token and see if it works.

@jasonwilliams jasonwilliams merged commit ce0d801 into boa-dev:master Jun 1, 2020
@jasonwilliams
Copy link
Member

Lets see what happens

@jasonwilliams
Copy link
Member

jasonwilliams commented Jun 1, 2020

@Lan2u
Copy link
Author

Lan2u commented Jun 1, 2020

@Lan2u
Copy link
Author

Lan2u commented Jun 1, 2020

Seems to be fixed, Pull request #436

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code coverage report
3 participants