MrCreosote commented 3 years ago

Looks like tests are failing

dcchivian commented 3 years ago

@jsfillman sounds good. Looks like the old log expired, but I suspect the current errors in the SDK unit tests are the same as the old commit. There appear to be 5 unit tests that are failing. https://github.com/kbaseapps/DataFileUtil/pull/72/checks?check_run_id=1477112011

dcchivian commented 3 years ago

4 errors/failures related to ftp tests (maybe the data on that path has been removed?) and one that appears to be a noclobber with a repeat path from another test

dcchivian commented 3 years ago

some of the tests are failing trying to do an ftp from ftp.uconn.edu. that address does not ping. not sure why that server was picked, but maybe we can test against a server from NCBI? Need to dig more to see whether the type of data matters...

dcchivian commented 3 years ago

some of the tests are failing trying to do an ftp from ftp.uconn.edu. that address does not ping. not sure why that server was picked, but maybe we can test against a server from NCBI? Need to dig more to see whether the type of data matters...

looks some of the tests actually want to save the test file that is subsequently downloaded. not sure what server we can use for that. thoughts?

codecov[bot] commented 3 years ago

Codecov Report

Merging #72 (dd6a938) into master (e36bf01) will decrease coverage by 0.08%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
- Coverage   70.30%   70.21%   -0.09%     
==========================================
  Files           3        3              
  Lines        1155     1155              
==========================================
- Hits          812      811       -1     
- Misses        343      344       +1

Impacted Files	Coverage Δ
lib/DataFileUtil/DataFileUtilImpl.py	`91.23% <0.00%> (-0.15%)`	:arrow_down:

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 e36bf01...dd6a938. Read the comment docs.

dcchivian commented 3 years ago

unit tests are passing. travis-ci is failing on "make sdkbase". is the travis base image not using a pre-built kbase image? @MrCreosote @jsfillman @Tianhao-Gu @qzzhang how should we proceed? My feeling is the travis-ci is an issue that should be fixed, but seems beyond the scope of this PR for DataFileUtil. If you agree, then @MrCreosote should probably merge. Gavin, do you agree?

(also, the RELEASE_NOTES.md has been updated, which Gavin identified as needing to happen)

MrCreosote commented 3 years ago

I think we wanted to merge this PR to get the release notes correct before the 0.1.3 bump: https://github.com/kbaseapps/DataFileUtil/pull/73

@qzzhang There's an open question for you - can you take a look?

IMO we should have the tests passing before we merge. It looks like they did pass at least once since we got code coverage, so it should be doable.

MrCreosote commented 3 years ago

From https://github.com/kbaseapps/DataFileUtil/commits/master, it looks like the tests broke here: https://github.com/kbaseapps/DataFileUtil/commit/c3a61c91e376af4af8c67661e6c2f7eca0bee01d

dcchivian commented 3 years ago

I think we wanted to merge this PR to get the release notes correct before the 0.1.3 bump: #73

@qzzhang There's an open question for you - can you take a look?

I think @Tianhao-Gu included the RELEASENOTES.md that @qzzhang put into #73, so merging #73 would be redundant. However, she should confirm that note on the : to filename change belongs with 0.1.1

dcchivian commented 3 years ago

From https://github.com/kbaseapps/DataFileUtil/commits/master, it looks like the tests broke here: c3a61c9

looking at the tests from that commit, it's not travis-ci that was broken back then rather the SDK unit tests that are passing now. however, we still need to fix the current travis-ci issue with "make sdkbase". I have no idea why it's building from scratch. is it always supposed to do that? @jsfillman @MrCreosote do you happen to know?

MrCreosote commented 3 years ago

@jsfillman Do we even need travis anymore or is it all covered by GHA?

dcchivian commented 3 years ago

@MrCreosote @jsfillman @Tianhao-Gu @qzzhang so the remaining error is in travis-ci and seems to be connected to blobstore. Is that a blocker to merging? Guessing yes since Gavin appears to be actively working on it, but please confirm.

MrCreosote commented 3 years ago

The last build that completed showed the make sdkbase error: https://travis-ci.org/github/kbaseapps/DataFileUtil/builds/748460971

MrCreosote commented 3 years ago

couple of follow on issues for this PR: https://github.com/kbaseapps/DataFileUtil/issues/80 https://github.com/kbaseapps/DataFileUtil/issues/79

kbaseapps / DataFileUtil

bump version to 0.1.3 to permit release on CI #72

Codecov Report