Test: Deprecate the 'expected' argument of QUnit.test#885
Conversation
There was a problem hiding this comment.
This is a great start for #861, but we need to make a better structure out of it.
QUnit 1.x.x still supports browsers and other environments without the console object, or not even the console.warn.
I believe we should make a better structure first before triggering the warnings, like prefixing all the deprecation messages with an all caps DEPRECATED, also the HTML Reporter might get this info from the logs and show it on the reporter messages as well.
Also, I would like to have an internal method for the warning messages instead of checking for console.warn everywhere.
There was a problem hiding this comment.
Thanks for the review.
like prefixing all the deprecation messages with an all caps DEPRECATED
Done.
also the HTML Reporter might get this info from the logs and show it on the reporter messages as well.
I'm not sure my design is the best one, but I came up with the following:
- Add a QUnit.deprecationWarnings list
- In deprecateWarns helper (added in core/utilities.js), use console.warn if available and add warnings to QUnit.deprecationWarnings
- In HTML reporter, use QUnit.deprecationWarnings to show warning list and total count of warnings(e.g. Tests completed in 1092 milliseconds. 511 assertions of 513 passed, 2 failed.1 warnings)
Does that sounds good to you?
Also, I would like to have an internal method for the warning messages instead of checking for console.warn everywhere.
Done. Added in core/utilities.js.
There was a problem hiding this comment.
Does that sounds good to you?
It sounds great, but I would use a QUnit.warnings instead of deprecationWarnings, with that we can reuse it to any other warning type, even if it's not a deprecated one.
Done. Added in core/utilities.js.
Did you pushed the commit with it? I would love to see it.
There was a problem hiding this comment.
Did you pushed the commit with it? I would love to see it.
No, I was waiting for your feedback for my other comments. I will update the PR in a day or two.
Thanks!
There was a problem hiding this comment.
This warnings API will solve #881 partially, and it'll be crucial for the 2.0 release 👍
|
Hey @berkerpeksag, I'm looking forward to see this when it's ready. |
|
My apologies, I have a job interview this week. I will work on the PR over the weekend. |
|
No problem. Thanks for the update and the best of luck on the interview. |
|
Sorry about the delay. I'm not so sure about storing all warnings in a global |
|
we got a conflict with #918, I need to figure out how to solve it. |
|
@berkerpeksag I'm sorry for the delay and mess on this process. The warning api is not going to land for this deprecation. Thanks for the support. |
Fixes #356
I left asyncTest untouched because of #656.