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

Test: Deprecate the 'expected' argument of QUnit.test #885

Closed
wants to merge 2 commits into
from

Conversation

3 participants
@berkerpeksag

Fixes #356

I left asyncTest untouched because of #656.

@leobalter

View changes

Show outdated Hide outdated src/test.js
if ( global.console && global.console.warn ) {
global.console.warn(
"'expected' argument of QUnit.test is deprecated. " +
"Call the expect() function explicitly instead."

This comment has been minimized.

@leobalter

leobalter Oct 25, 2015

Member

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.

@leobalter

leobalter Oct 25, 2015

Member

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.

This comment has been minimized.

@berkerpeksag

berkerpeksag Oct 28, 2015

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.

@berkerpeksag

berkerpeksag Oct 28, 2015

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.

This comment has been minimized.

@leobalter

leobalter Dec 9, 2015

Member

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.

@leobalter

leobalter Dec 9, 2015

Member

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.

This comment has been minimized.

@berkerpeksag

berkerpeksag Dec 9, 2015

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!

@berkerpeksag

berkerpeksag Dec 9, 2015

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!

This comment has been minimized.

@leobalter

leobalter Dec 9, 2015

Member

This warnings API will solve #881 partially, and it'll be crucial for the 2.0 release 👍

@leobalter

leobalter Dec 9, 2015

Member

This warnings API will solve #881 partially, and it'll be crucial for the 2.0 release 👍

This comment has been minimized.

@leobalter

View changes

Show outdated Hide outdated test/main/test.js
QUnit.test( "expected of QUnit.test is deprecated", 42, function( assert ) {
assert.expect( 0 );
});

This comment has been minimized.

@leobalter

leobalter Oct 25, 2015

Member

This test triggers a false positive for the changes coverage. We need to actual test if it's warning anything. That's where having a warning details on the log might help.

@leobalter

leobalter Oct 25, 2015

Member

This test triggers a false positive for the changes coverage. We need to actual test if it's warning anything. That's where having a warning details on the log might help.

This comment has been minimized.

@berkerpeksag

berkerpeksag Oct 28, 2015

Agreed. Do you have an API design to test warnings. For example:

QUnit.test( "expected of QUnit.test is deprecated", 42, function( assert ) {
    assert.equal(
        assert.warnings[0],
        "'expected' argument of QUnit.test is deprecated." +
        "Call the expect() function explicitly instead."
    );
});
@berkerpeksag

berkerpeksag Oct 28, 2015

Agreed. Do you have an API design to test warnings. For example:

QUnit.test( "expected of QUnit.test is deprecated", 42, function( assert ) {
    assert.equal(
        assert.warnings[0],
        "'expected' argument of QUnit.test is deprecated." +
        "Call the expect() function explicitly instead."
    );
});

This comment has been minimized.

@leobalter

leobalter Dec 9, 2015

Member

Not only registering the warnings on a list/array, QUnit.log should include it.

@leobalter

leobalter Dec 9, 2015

Member

Not only registering the warnings on a list/array, QUnit.log should include it.

This comment has been minimized.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Dec 30, 2015

Member

Hey @berkerpeksag, I'm looking forward to see this when it's ready.

Member

leobalter commented Dec 30, 2015

Hey @berkerpeksag, I'm looking forward to see this when it's ready.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Jan 6, 2016

My apologies, I have a job interview this week. I will work on the PR over the weekend.

My apologies, I have a job interview this week. I will work on the PR over the weekend.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Jan 6, 2016

Member

No problem. Thanks for the update and the best of luck on the interview.

Member

leobalter commented Jan 6, 2016

No problem. Thanks for the update and the best of luck on the interview.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Feb 15, 2016

Sorry about the delay.

I'm not so sure about storing all warnings in a global QUnit.warnings array. Perhaps it should be stored as config.moduleStats.warnings or something similar?

Sorry about the delay.

I'm not so sure about storing all warnings in a global QUnit.warnings array. Perhaps it should be stored as config.moduleStats.warnings or something similar?

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Feb 16, 2016

Member

we got a conflict with #918, I need to figure out how to solve it.

Member

leobalter commented Feb 16, 2016

we got a conflict with #918, I need to figure out how to solve it.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Apr 18, 2016

Member

@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.

Member

leobalter commented Apr 18, 2016

@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.

@leobalter leobalter closed this Apr 18, 2016

@berkerpeksag berkerpeksag deleted the berkerpeksag:issue-356-expected branch Apr 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment