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

Moved validation test data out of large files area. #5381

Merged
merged 3 commits into from
Oct 31, 2018

Conversation

jonn-smith
Copy link
Collaborator

Now the validation test data sets are in the normal git file repository.
This allows them to be visually inspected for differences when they have
changed (during a code review).

Fixes #5379

Now the validation test data sets are in the normal git file repository.
This allows them to be visually inspected for differences when they have
changed (during a code review).

Fixes #5379
@lbergelson
Copy link
Member

@jonn-smith Something is weird here, it looks like the the stubs are committed now by accident. That's what I see if I check it out locally. They're still valid git-lfs stubs, so running a git-lfs checkout will pull them down and then git will think that they've all been modified since they're not being tracked correctly by git-lfs... In the past to move things out of git-lfs I've had to do it in 2 commits.

  1. remove the file from git,
  2. re-commit it to a new location that isnt tracked by git-lfs. git lfs migrate export might help, but I've never used it.

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

The state of the files is really wierd, should not be merged until it's resolve, see my other comment.

@droazen
Copy link
Contributor

droazen commented Oct 31, 2018

Good catch @lbergelson -- @jonn-smith could you fix this following Louis's instructions above? If you do it correctly, the diff on github should show that you've added thousands of lines.

@jonn-smith
Copy link
Collaborator Author

Gross. OK.

I thought a git mv would take care of all this.

I'll fix it up now.

Copy link
Contributor

@droazen droazen 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 now @jonn-smith -- merge once tests pass.

@droazen droazen dismissed lbergelson’s stale review October 31, 2018 18:37

Issue resolved

@droazen droazen assigned jonn-smith and unassigned droazen Oct 31, 2018
@jonn-smith jonn-smith merged commit a14917d into master Oct 31, 2018
@jonn-smith jonn-smith deleted the jts_move_expected_outs_5379 branch October 31, 2018 19:44
@codecov-io
Copy link

Codecov Report

Merging #5381 into master will increase coverage by <.001%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master     #5381       +/-   ##
===============================================
+ Coverage     86.933%   86.933%   +<.001%     
  Complexity     30327     30327               
===============================================
  Files           1849      1849               
  Lines         140801    140802        +1     
  Branches       15477     15477               
===============================================
+ Hits          122402    122403        +1     
  Misses         12735     12735               
  Partials        5664      5664
Impacted Files Coverage Δ Complexity Δ
...nder/tools/funcotator/FuncotatorTestConstants.java 97.727% <100%> (+0.053%) 1 <0> (ø) ⬇️

EdwardDixon pushed a commit to EdwardDixon/gatk that referenced this pull request Nov 9, 2018
)

* Moved validation test data out of large files area.

Now the validation test data sets are in the normal git file repository.
This allows them to be visually inspected for differences when they have
changed (during a code review).

Fixes broadinstitute#5379

* Removing test files.

* Adding back in test files outside of gitlfs to fix lfs file problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants