-
Notifications
You must be signed in to change notification settings - Fork 55
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
tests to use fallback exception to handle FieldError
exception
#539
Conversation
JuliaLang/julia#54504 is not merged yet. So all tests will fail. But we need these for original PR. So how do we handle this ? |
test/spqr.jl
Outdated
@@ -39,7 +39,7 @@ itypes = sizeof(Int) == 4 ? (Int32,) : (Int32, Int64) | |||
@test istriu(F.R) | |||
@test isperm(F.pcol) | |||
@test isperm(F.prow) | |||
@test_throws ErrorException F.T | |||
@test_throws FieldError F.T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@test_throws FieldError F.T | |
@test_throws Exception F.T |
This will work both before and after #54504 (suggested by Eric Hanson on slack)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this temporary solution ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also do @test_throws Union{FieldError, ErrorException} F.T
if we want to be a little more precise and only allow those 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. I didn't know we can do that. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't do that Union because FieldError does not exist. We could do @test_throws isdefined(Base, :FieldError) ? FieldError : ErrorException F.T
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I should have tested on local PC first. I tested it this time.
FieldError
exception insteadFieldError
exception
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #539 +/- ##
==========================================
- Coverage 84.07% 83.73% -0.34%
==========================================
Files 12 12
Lines 9068 9046 -22
==========================================
- Hits 7624 7575 -49
- Misses 1444 1471 +27 ☔ View full report in Codecov by Sentry. |
with PR#54504 field access exception is raised as `FieldError` exception now.
CI tests pass, though codecov upload fails. I see a similar pattern on other merged PRs. I'm assuming it's just the codecov setup being unreliable. |
with #54504 field access exception is raised as
FieldError
exception now.