-
Notifications
You must be signed in to change notification settings - Fork 32
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
Expand JET test #782
Expand JET test #782
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #782 +/- ##
=======================================
Coverage 86.16% 86.16%
=======================================
Files 36 36
Lines 4301 4301
=======================================
Hits 3706 3706
Misses 595 595 ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 12952966615Details
💛 - Coveralls |
Hmm, I don't think I fully understand the change 🤔 The intent with the original test is to the check that I think if we want the error itself, then we should just print that information upon test failure instead ,no? I.e. just add an |
Aha, okay, I see what you mean. I was looking at this because there was a case where Is that expected? |
It's not un-intended 👀 I guess I just didn't think of it, but I do think we want to keep it as is so make sure that the |
Sure, the main thing I think I'd have liked to know was why it didn't determine a typed varinfo. I'm happy to add that in rather than changing this one, I just wasn't sure if that was what this was intended to be heh. |
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.
Lovely @penelopeysm !
I'm not sure if this is the right thing to do, so pinging @torfjelde:
If JET infers an untyped varinfo then calling
test_call
later with an untyped varinfo doesn't tell us why it did so - instead we have to call it with a regular typed VarInfo - I think? Let me know if your intent was otherwise:)(CI on this PR will likely fail, precisely because JET is inferring an untyped varinfo for one of our demo models, there's more explanation here #781 (comment).)