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

Add More tests #101

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Add More tests #101

merged 1 commit into from
Dec 18, 2020

Conversation

programmeroftheeve
Copy link
Contributor

@programmeroftheeve programmeroftheeve commented Dec 16, 2020

Adds tests for the additional load functions.
Contains commits from #100

@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #101 (e58bcc1) into master (52ef020) will increase coverage by 1.42%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
+ Coverage   81.32%   82.75%   +1.42%     
==========================================
  Files          12       12              
  Lines        1462     1461       -1     
==========================================
+ Hits         1189     1209      +20     
+ Misses        273      252      -21     
Impacted Files Coverage Δ
src/scanner.jl 83.40% <0.00%> (+0.40%) ⬆️
src/YAML.jl 91.17% <0.00%> (+52.94%) ⬆️

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 52ef020...e58bcc1. Read the comment docs.

@programmeroftheeve
Copy link
Contributor Author

Ok, so I am glad it wasn't my multi-constructor code from earlier. But I am confused on why running load_all fails on empty_scaler.yml reading from the read string on CI only.
It would have to something weird with IOBuffer.

test/runtests.jl Outdated Show resolved Hide resolved
@kescobo
Copy link
Collaborator

kescobo commented Dec 16, 2020

Thanks for digging into it! Definitely weird - here's the first bit of the stacktrace (I'm sure you've seen it, just pulling it out for others)

Load All from String: Error During Test at D:\a\YAML.jl\YAML.jl\test\runtests.jl:168
  Test threw exception
  Expression: equivalent(first(allStringData), expected)
  AssertionError: typeof(forward!(composer.input)) == DocumentStartEvent
  Stacktrace:
   [1] compose_document(::YAML.Composer) at D:\a\YAML.jl\YAML.jl\src\composer.jl:49
   [2] compose(::YAML.EventStream) at D:\a\YAML.jl\YAML.jl\src\composer.jl:38

Have you pulled out and looked at what first(allStringData) is and compared it with expected? I understand it's tough to debug given that it's not happening locally, but even throwing in some print statements might help

@programmeroftheeve
Copy link
Contributor Author

@kescobo So after putting in debug statements and trying some data massaging, the empty_scalar doesn't like a new line at the end of the string. I am not sure why this is, but it is a place to start.

@kescobo
Copy link
Collaborator

kescobo commented Dec 17, 2020

Windows often uses different line endings (\r I think) - not sure why it would only happen on the ci machine, but I'd be content to chomp the line endings and just compare the line contents if that's possible for this test (on phone so can't easily check). I'm pretty sure julia's rstrip can do this.

Chomp the strings for testing
@programmeroftheeve
Copy link
Contributor Author

Chomped the strings. Everything passes. Yay! 😁

@kescobo
Copy link
Collaborator

kescobo commented Dec 18, 2020

Thank you! I'll try to review and merge tomorrow

Copy link
Collaborator

@kescobo kescobo left a comment

Choose a reason for hiding this comment

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

Looks great to me - I'll open an issue about this line ending problem - should be possible to do platform-dependent tests so that we don't have to chomp line endings, but this is good for now I think

@kescobo kescobo merged commit 36128a7 into JuliaData:master Dec 18, 2020
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.

2 participants