-
Notifications
You must be signed in to change notification settings - Fork 214
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
round factor tests #7515
base: master
Are you sure you want to change the base?
round factor tests #7515
Conversation
You may need to run rush change, based on the CI build errors - these are updates to the tests, so I would just leave the changelog comments blank |
I'll defer to @rschili if he believes this is enough test cases |
}; | ||
|
||
const format = new Format("test"); | ||
await format.fromJSON(unitsProvider, formatData).catch(() => { }); |
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.
why catch here? Test should fail, so if the exception is thrown that should be fine?
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.
hmm good point. This is how format is set up in all other tests as well. I guess it means we wanna proceed with whatever information was successfully loaded into the format (even in the future they change impl of loading format), rather than failing immediately due to an exception. So allow testing with partial info? it is a bit weird.
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.
hm can you quickly verify that's the case? If it is, that seems like a wonky test, relying on things that are not allowed out there in the wild to compose the test inputs...
If there is no error thrown, please remove the portion
#7462