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

Apidsl fixes and start tracking test coverage #1

Merged
merged 5 commits into from
Jul 7, 2016

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jul 6, 2016

This change is Reviewable

@coveralls
Copy link

coveralls commented Jul 6, 2016

Coverage Status

Changes Unknown when pulling 71860a3 on iphydf:master into * on TokTok:master*.

@iphydf iphydf force-pushed the master branch 6 times, most recently from acff665 to 071b4b3 Compare July 6, 2016 12:28
@coveralls
Copy link

coveralls commented Jul 6, 2016

Coverage Status

Changes Unknown when pulling 071b4b3 on iphydf:master into * on TokTok:master*.

@coveralls
Copy link

coveralls commented Jul 6, 2016

Coverage Status

Changes Unknown when pulling 071b4b3 on iphydf:master into * on TokTok:master*.

@nurupo
Copy link
Member

nurupo commented Jul 6, 2016

@iphydf any reason why @DeriveFH (irungentoo#1542) is not attributed for Fixes for apidsl?

@nurupo
Copy link
Member

nurupo commented Jul 6, 2016

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.


.travis.yml, line 2 [r1] (raw file):

sudo: required
dist: trusty

Why are we switching Travis away from using Ubuntu 14.04 to Ubuntu 12.04? https://docs.travis-ci.com/user/ci-environment/#Virtualization-environments

Ubuntu 14.04 has libconfig-dev, libvpx-dev, libopus-dev and check in it's package repos, which eliminates all the building you do below in this file.


.travis.yml, line 17 [r1] (raw file):

cache:
  directories:
    - $HOME/cache

Hm, you want to cache all the builds to save time it takes for Travis to run? It looks okay.


.travis.yml, line 73 [r1] (raw file):

        cd ..
      }
  - popd

Most of this building is not necessary if Ubuntu 14.04 is used.


.travis.yml, line 90 [r1] (raw file):

        CFLAGS="-O0 -Wall -Wextra -fprofile-arcs -ftest-coverage"
  - make
  - make check || true

Doesn't Travis already fail a build if any of commands exit with non 0 code?


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Jul 6, 2016

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.


.travis.yml, line 90 [r1] (raw file):

Previously, nurupo wrote…

Doesn't Travis already fail a build if any of commands exit with non 0 code?

Ok, misread this. So, we ignore the exit code of the testing?

Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Jul 6, 2016

I didn't know that PR existed when I made my changes, but I'm happy to incorporate commits from there. I'll back out my changes.


Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.


.travis.yml, line 2 [r1] (raw file):

Previously, nurupo wrote…

Why are we switching Travis away from using Ubuntu 14.04 to Ubuntu 12.04? https://docs.travis-ci.com/user/ci-environment/#Virtualization-environments

Ubuntu 14.04 has libconfig-dev, libvpx-dev, libopus-dev and check in it's package repos, which eliminates all the building you do below in this file.

This is because sudo enabled builds don't support caching. It saves quite a lot of time to avoid building apidsl dependencies and libsodium every time.

.travis.yml, line 17 [r1] (raw file):

Previously, nurupo wrote…

Hm, you want to cache all the builds to save time it takes for Travis to run? It looks okay.

Indeed.

.travis.yml, line 73 [r1] (raw file):

Previously, nurupo wrote…

Most of this building is not necessary if Ubuntu 14.04 is used.

Correct, but the building is mitigated by caching which is not available on 14.04.

.travis.yml, line 90 [r1] (raw file):

Previously, nurupo wrote…

Ok, misread this. So, we ignore the exit code of the testing?

Yes, we ignore test outcome because it fails. The first thing we need to do after this is to fix the test and stop ignoring its failure.

Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Jul 6, 2016

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.


.travis.yml, line 73 [r1] (raw file):

Previously, iphydf wrote…

Correct, but the building is mitigated by caching which is not available on 14.04.

This page says that caching is available in 14.04 https://docs.travis-ci.com/user/ci-environment/#Virtualization-environments

I guess whether it's available or not doesn't matter because the boot time for 14.04 is way slower than for the container 12.04.


Comments from Reviewable

- We use coveralls.io to report on test coverage and avoid getting below a
  certain threshold. The threshold is currently 60%, but we will be increasing
  it when it stabilises.
- We use gcc/clang -ftest-coverage and gcov to measure C test coverage.
- We switched to container based Travis build infrastructure, which has the
  advantage of faster boot times[1] (1-6s vs. 20-52s). The trusty beta supports
  caching, but the longer boot times make it an unattractive target.
- We now need to build more dependencies ourselves and cache the result. We
  still fetch what we can (currently opam, libvpx, and check) from apt.

[1] https://docs.travis-ci.com/user/ci-environment/#Virtualization-environments
@iphydf
Copy link
Member Author

iphydf commented Jul 7, 2016

@yashikno I've merged your changes here. Thanks for making them.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 83f5258 on iphydf:master into * on TokTok:master*.

@iphydf
Copy link
Member Author

iphydf commented Jul 7, 2016

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.


.travis.yml, line 73 [r1] (raw file):

Previously, nurupo wrote…

This page says that caching is available in 14.04 https://docs.travis-ci.com/user/ci-environment/#Virtualization-environments

I guess whether it's available or not doesn't matter because the boot time for 14.04 is way slower than for the container 12.04.

Amended the commit message.

Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Jul 7, 2016

How/where is the 60% of code coverage enforced?

@iphydf
Copy link
Member Author

iphydf commented Jul 7, 2016

Unfortunately, that is currently only set on the coveralls settings page. I hope to get codecov.io submission working at some point, and then the setting is in a codecov.yml file.

@nurupo
Copy link
Member

nurupo commented Jul 7, 2016

Reviewed 1 of 3 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Jul 7, 2016

Hm, reviewable shows me a "Merge & Retain Branch", which is interesting as I shouldn't have permissions to push into this repo.

@iphydf iphydf merged commit 83f5258 into TokTok:master Jul 7, 2016
@nurupo
Copy link
Member

nurupo commented Jul 7, 2016

Yet GitHub says
dfdfsdf

I guess reviewable doesn't check if I have permission until I try to merge through its web UI and GitHub API returns an error.

iphydf referenced this pull request in iphydf/c-toxcore Sep 6, 2016
Fix #1: No longer allow clients to create multiple instances with the same chat_id
Fix #2: Properly timeout group join attempts even when not connected to the DHT
@iphydf iphydf modified the milestone: v0.0.1 Nov 6, 2016
@tox-user tox-user mentioned this pull request Jan 29, 2018
@jackzhp jackzhp mentioned this pull request Aug 11, 2018
tvladyslav pushed a commit to tvladyslav/c-toxcore that referenced this pull request Oct 27, 2018
iphydf pushed a commit that referenced this pull request May 2, 2020
iphydf pushed a commit that referenced this pull request May 2, 2020
iphydf pushed a commit that referenced this pull request May 3, 2020
iphydf pushed a commit that referenced this pull request May 4, 2020
iphydf pushed a commit that referenced this pull request May 5, 2020
iphydf pushed a commit that referenced this pull request May 8, 2020
iphydf pushed a commit that referenced this pull request May 17, 2020
iphydf pushed a commit that referenced this pull request May 17, 2020
restyled-io bot pushed a commit that referenced this pull request Jul 7, 2020
JFreegman pushed a commit that referenced this pull request Aug 11, 2020
restyled-io bot pushed a commit that referenced this pull request Nov 17, 2021
restyled-io bot pushed a commit that referenced this pull request Nov 30, 2021
This pull request was closed.
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.

4 participants