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

#1175 automate code coverage #1176

Merged
merged 49 commits into from
Jun 14, 2019
Merged

Conversation

macgills
Copy link
Contributor

Fixes #1175

Changes: Integrate with codecov, fix broken automation tests

Screenshots/GIF for the change: [If possible, please add relevant screenshots/GIF]

@codecov
Copy link

codecov bot commented May 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (macgills/2.5-kotlin@fac411f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                  Coverage Diff                   @@
##             macgills/2.5-kotlin    #1176   +/-   ##
======================================================
  Coverage                       ?   34.85%           
  Complexity                     ?        1           
======================================================
  Files                          ?      128           
  Lines                          ?     5032           
  Branches                       ?      584           
======================================================
  Hits                           ?     1754           
  Misses                         ?     3075           
  Partials                       ?      203

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fac411f...38020ed. Read the comment docs.

@macgills macgills requested a review from mhutti1 May 30, 2019 13:03
@macgills
Copy link
Contributor Author

So the end result of this PR is

  • full coverage integration with our unit tests
  • a commented out hopeful integration with our instrumentation tests to be integrated with once the next version of kiwixlib is published
  • building of releasable apks with generated versionNames, versionCodes and abi splits
  • the ability to publish to GitHub releases (as a draft)
  • the ability to publish to play store alpha with a source controlled listing see src/kiwix/play/*

I am still a bit unsure about the play store publishing because I want it to happen on the tagging of release (and in the future master) but I think to allow that to happen we would have to toggle the travis setting "Build pushed branches" and that would have further ramifications that I would need to address potentially by combining conditional stages/jobs. For now I am going to operate on the hope that it will just build when a tag is created and cross that bridge when we come to it

@soloturn
Copy link
Contributor

soloturn commented Jun 1, 2019

Sorry I should have tagged the title of this PR as Do Not Review or something similar.

ah, sure! no problem at all. sorry for my premature statement.

... when the time comes to review, definitely not look at the individual commits

i am not sure if i understand this sentence correctly. how do you plan to get these commits onto master if you are not able to separate your work into digestable pieces, where each if the pieces makes sense, and has appropriate quality? or this is just a test, and you write the code again?

@macgills
Copy link
Contributor Author

macgills commented Jun 4, 2019

@soloturn we are moving to using merge commits for PRs, this means the commits will no longer be replayed on master, just one merge commit totalling the overall changes of the branch so master's history will not look sloppy. I have never personally in my career found looking at the individual commits very useful in the review process, a class can go through many iterations before I arrive on my preferred state and to read it would just be confusing, in rare scenarios commits can shed some extra light but I only found that for much larger line changes than this one.
A lot of this work is a bit untidy as I start aligning to the process we will adhere to going forward. By the time of 3.0 it will hopefully be a lot clearer

@soloturn
Copy link
Contributor

soloturn commented Jun 4, 2019

interesting. you have such an example already merged? would be curious to look how the commit history and the commit graph then looks like, as well the commit message. i would really love to see e.g. this one merged to your other branch, and then merged to master for example.

i tried to read the changed files list. 55 files. functional changes, documentation changes, reformatting, increase versions, slightly different syntax in the gradle file. inside there are small things like one time lynx --dump with one and two dashes - not sure if deliberate or not. not easy to read, not easy to approve. where you are coming from is clear, committing the same way, especially in my day job :) so - if what you say works well i could safe some time, no more squashing and splitting commits to facilitate the commit readers life.

@macgills
Copy link
Contributor Author

macgills commented Jun 5, 2019

image

Here is an example of the "Introduce Unit Testing" ticket being merged, as a branch naming scheme I think the number in the branch name and per commit gives good context when viewing history, my experience is primarily in JIRA/Bitbucket so I don't know if this will create unnecessary chaff with GitHub but I don't think so.
https://nvie.com/posts/a-successful-git-branching-model/ this is the end result I am hoping to achieve with 3.0, unfortunately 2.5's isolated nature means some workarounds at the moment.
A more comment heavy approach may be necessary, in the example of --dump vs -dump absolutely an error by myself and should be fixed, I will commit momentarily. As for the 55 files at least 40 of them can be ignored as they are bootstrapped files mirroring our play store listing

@macgills
Copy link
Contributor Author

@soloturn @mhutti1 this PR is now blocking the 2.5 release. If it could be reviewed asap that would be appreciated but in the interest of expediency if I do not have approval by tomorrow I will go ahead with the merge and move on to merging 2.5 to the release branch

Copy link
Contributor

@mhutti1 mhutti1 left a comment

Choose a reason for hiding this comment

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

Looks good

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