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

use github action for unittest #52

Merged
merged 9 commits into from
Jul 15, 2020

Conversation

johnnychen94
Copy link
Contributor

@johnnychen94 johnnychen94 commented Jul 12, 2020

Github Action can run more jobs in parallel, which makes it faster
to fully cover most platform and julia versions

In the meantime, still use travis to submit code coverage report

I have submitted many PRs in parallel, this one conflicts with others and should be merged in the last.

Github Action can run more jobs in parallel, which makes it faster
to fully cover most platform and julia versions

In the meantime, still use travis to submit code coverage report
@johnnychen94
Copy link
Contributor Author

johnnychen94 commented Jul 12, 2020

I'm quite confused that LinearMapsAA is incompatible with Julia 1.3 JeffFessler/LinearMapsAA.jl#15. But to keep this PR simple and not involved with other issues, I just set the CI to Julia 1.4.

Currently, "1" is equivalent to "1.4", which is a duplication. But given that Julia 1.5.0 is going to be released in a month, I think it's okay here.

@codecov
Copy link

codecov bot commented Jul 12, 2020

Codecov Report

Merging #52 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #52   +/-   ##
=======================================
  Coverage   98.86%   98.86%           
=======================================
  Files          48       48           
  Lines        2107     2107           
=======================================
  Hits         2083     2083           
  Misses         24       24           

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 7a84f69...9fd7f7d. Read the comment docs.

@johnnychen94
Copy link
Contributor Author

johnnychen94 commented Jul 12, 2020

Apparently it caught some filepath issues by bringing windows back.

@JeffFessler
Copy link
Owner

There were some merge conflicts with this so I tried to resolve them - hopefully correctly, but I'm unsure since I don't know anything about unittest. I'll wait to see what happens with the tests...

README.md Outdated Show resolved Hide resolved
@JeffFessler JeffFessler mentioned this pull request Jul 15, 2020
README.md Show resolved Hide resolved
@JeffFessler
Copy link
Owner

@johnnychen94 hopefully I "fixed" (worked around) the problem with windows failing.
Probably you need to merge in the latest from master into this branch and then see if the tests pass?
(Maybe I could do that myself but I don't know how...)

@johnnychen94
Copy link
Contributor Author

johnnychen94 commented Jul 15, 2020

This PR is growing with unrelated commits, to keep commit history clean, it would be better to merge with squash.

Given that this PR doesn't touch the MIRT functionality, another strategy is to first merge this PR into master and then submit fixes for windows in later PRs. That way we don't need to frequently pull/rebase between multiple branches.

Hopefully fld_write is the only thing broken in Windows...

(Maybe I could do that myself but I don't know how...)

I don't know how to do it locally, either.

@JeffFessler JeffFessler merged commit 7846f0f into JeffFessler:master Jul 15, 2020
@JeffFessler
Copy link
Owner

Thanks for all the advice!

@JeffFessler
Copy link
Owner

Some unit tests are failing after I restored the time/ directory:
https://github.com/JeffFessler/MIRT.jl/actions/runs/170243628
That directory should not be tested ever so I'm not sure what is the issue...

@johnnychen94 johnnychen94 deleted the jc/unittest branch July 15, 2020 18:12
@johnnychen94
Copy link
Contributor Author

It looks like a random platform failure to me. GitHub isn't very reliable recently...

@JeffFessler
Copy link
Owner

Currently, "1" is equivalent to "1.4", which is a duplication. But given that Julia 1.5.0 is going to be released in a month, I think it's okay here.

@johnnychen94 to reduce carbon footprint, for now i am going to comment out some tests and just test 1.4 on linux. if there is some important reason to test it on other OS let me know. i already test it on my mac anyway.

@johnnychen94
Copy link
Contributor Author

johnnychen94 commented Jul 21, 2020

There're three aspects that I have in mind wrt this:

  • [quality control] CI is used to early detect possible regression and bugs. If these get disabled, then we lost track of how new Julia versions affect this package unless we manually test it, which can be a disaster in practice.
  • [collaboration] I noticed that this package is mostly maintained by yourself in a commit-and-push manner, which is okay when there's only one developer. But if there's a tendency to switch to the PR-and-merge manner, which is used widely to collaboration, IMO a larger coverage in CI should be a must. -- Generally speaking, we don't trust codes from others unless CI passes.
  • [advertisement] I guess this package is used mainly for internal usage now. In some sense, CI coverage helps attract more people outside by telling them "Hey! I've tested that this package works perfectly on your settings and please take a try" 😄

If you think this is accepted, then we're good. I don't have a strong opinion on this because this package is mostly maintained by you. Just that I personally don't think this is a good tradeoff.

@JeffFessler
Copy link
Owner

Ok, that is compelling. Thanks for the nice explanations.
I have been using PR-and-merge in my more recent work.
I will restore the extra OS tests soon based on your advice!
(But testing 1 and 1.4 just seem to duplicate tests unnecessarily so I will avoid that for now unless there is some benefit that I am missing. When 1.5 is out then certainly we can update.)

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.

None yet

2 participants