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 tests for printf.jl #15377

Merged
merged 1 commit into from
Mar 9, 2016
Merged

Add tests for printf.jl #15377

merged 1 commit into from
Mar 9, 2016

Conversation

thepulkitagarwal
Copy link
Contributor

Since the error has to deal with the number of arguments, ArgumentError is more apt.

Also, in the same file, a similar statement (on Line 1167) is present:

throw(ArgumentError($macroname,": wrong number of arguments (",length(G),") should be (",$(length(sym_args)),")"))

@tkelman
Copy link
Contributor

tkelman commented Mar 7, 2016

makes sense to me

@thepulkitagarwal
Copy link
Contributor Author

#14410 might be relevant. I apologize for not seeing this earlier.

@tkelman
Copy link
Contributor

tkelman commented Mar 7, 2016

It's okay. I think the author of that PR changed his mind about wanting to contribute, but if you'd like to take what he started with, rebase and clean it up for re-submission that would be welcome.

@thepulkitagarwal
Copy link
Contributor Author

I'd like to do that. Thank you!

@thepulkitagarwal thepulkitagarwal changed the title Change generic error to ArgumentError in printf.jl Add tests for printf.jl Mar 7, 2016
@thepulkitagarwal
Copy link
Contributor Author

I added the tests, based on those in #14410.

@thepulkitagarwal
Copy link
Contributor Author

I figured out the problem:

@sprintf("%p",42)
# returns 0x000000000000002a in a 64 bit system

which is 64 bits, but in a 32 bit system, it will return a 32 bit hex code. So, the tests will fail.

I checked the tests on integers and removed all tests that presumed 64 bits. But I need some help for floats. @tkelman Can you tell me what I should do to check that?

Thank you!

@StefanKarpinski
Copy link
Member

Floats don't depend on system word size so those should be safe to leave unconditional. Like @tkelman suggested, the simplest way to fix the tests on 32-bit is to make the value you compare against conditionl on WORD_SIZE.

@thepulkitagarwal
Copy link
Contributor Author

Okay! I have added the pointer tests based on the word size.

Also, I want to know what parts of code I should cover next. The manual method in #9493 for checking code coverage gives a very different result from Codecov. What should I do to get more accurate data?

("%-20a","0x2.ap+4 "),
("%f", "42.000000"),
("%g", "42")),
num in (UInt16(42), UInt32(42), UInt64(42), UInt128(42),
Copy link
Contributor

Choose a reason for hiding this comment

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

is num getting used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was supposed to be added in Line 30 (instead of 42). I'll add it now!

@thepulkitagarwal
Copy link
Contributor Author

I have added a few more tests (two, to be exact) which were not covered earlier. I can rebase all the commits in this pull request into one commit before merging.

Also, is there anything else I should do?

Thank you!

@tkelman
Copy link
Contributor

tkelman commented Mar 8, 2016

Nevermind my most recent comment which I deleted, I see there was a duplicated line in the existing test.

I think all this needs is a squash, nice work!

@thepulkitagarwal
Copy link
Contributor Author

@tkelman I squashed all the commits. Can you check if everything's alright?

tkelman added a commit that referenced this pull request Mar 9, 2016
@tkelman tkelman merged commit 597dc7b into JuliaLang:master Mar 9, 2016
@tkelman
Copy link
Contributor

tkelman commented Mar 9, 2016

Thanks!

@tkelman tkelman added the test This change adds or pertains to unit tests label Mar 9, 2016
@thepulkitagarwal thepulkitagarwal deleted the printf.jl branch March 9, 2016 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants