All: Remove deprecated features by leobalter · Pull Request #976 · qunitjs/qunit

leobalter · 2016-03-25T22:47:53Z

globals
assertions on QUnit namespace
QUnit.stop
QUnit.init
setup/teardown
asyncTest
test with expected and async parameters
old style for logging callbacks
duration property on testDone details

This is the first batch. It can be improved and QUnit.reset is not addressed here yet.

gibson042 · 2016-03-26T00:34:48Z

src/core.js

-	asyncTest: asyncTest,
+	asyncTest: function() {
+		throw new Error(
+			"asyntTest is now removed from QUnit 2.0, check out our upgrade guide at " +


s/asyntTest/asyncTest/. And as an aside, I notice that only some of the removals come with throws... when will we fully complete the process and drop even these?

I've set throws only for removed methods as asyncTest, init, stop, and the previously exposed globals.

I skipped throwing for features as setup/teardown, test signature, etc.

This should mention QUnit.test() with assert.async() as replacement:

"asyncTest is removed in QUnit 2.0, use QUnit.test() with assert.async() instead. Details in our upgrade guide at " +

Then apply that to any other replacements as well.

leobalter · 2016-04-12T18:43:56Z

@gibson042 is there anything I should change?

@jzaefferer would you take a look on this one?

trentmwillis · 2016-04-13T00:24:58Z

src/export.js

-	}
+( function() {
+	var i;
+	var assertions = Assert.prototype;


Should use a single var

TL;DR: yes.

that's a mess I'm making after changes that happened on our style guide. It was a suggesting a single var and then it suggested 1 var per line, stacking non initialized variables on the same line. Now there's nothing and I'm confused.

At least on of the examples the style guide is using a single var, so I'll apply the change you suggested.

Okay. I personally prefer 1 var per line, but everywhere else in QUnit we use a single var.

done! I fixed this on a rebased commit, my bad I amended to a fixup commit.

trentmwillis · 2016-04-13T00:26:05Z

Overall 👍. Still need to get that typo mentioned here.

- globals - assertions on QUnit namespace - QUnit.stop - QUnit.init - setup/teardown - asyncTest - test with expected and async parameters - old style for logging callbacks - duration property on testDone details

leobalter · 2016-04-13T02:42:30Z

I'll wait for @jzaefferer to take a look on this, as he mentioned he will do it tomorrow, or today on his timezone. :)

After it's merged I'll proceed merging the other done PRs with breaking changes.

I'll probably try a pre-release so we can all test it on different suites.

jzaefferer · 2016-04-13T11:24:54Z

src/core.js

-		// DEPRECATED: handles setup/teardown functions,
-		// beforeEach and afterEach should be used instead
-		if ( testEnvironment && testEnvironment.setup ) {
-			testEnvironment.beforeEach = testEnvironment.setup;


This should throw an error as well, otherwise the setup and teardown properties are simply ignored, breaking tests in unexpected ways.

We can't throw, otherwise removing it on 2.1 would be a breaking change.

We may log a warning, console.warn is available on the set of supported browsers for 2.0

gibson042 · 2016-04-13T16:42:01Z

src/export.js

+function applyDeprecated( name ) {
+	return function() {
+		throw new Error(
+			"`" + name + "` is removed in QUnit 2.0, you should now use the " +


Let's drop the backticks; this isn't a great error message:

The global ok is removed…

ef9c175 what do you think? Also the global message is "The global ok is removed..."

leobalter · 2016-04-13T17:27:14Z

@jquery/qunit I believe I addressed all the reviews so far, how is it now?

gibson042 · 2016-04-13T20:51:52Z

src/test.js

+	config.current.semaphore -= 1;
+
+	// If semaphore is non-numeric, throw error
+	if ( isNaN( config.current.semaphore ) ) {


This is probably unnecessary now that the function is internal.

semaphore is still exposed. Can we remove it securely? I'll check this.

If so, I can do this (es5 + ie9+ support ftw):

Object.defineProperty(config.current, "semaphore", { set: function() { throw ... } ));

semaphore is not documented, so anyone using it directly is already coloring outside the lines.

21c254a I tried this refactoring, it's working and it avoids using the config.current using test directly whenever is possible. I'll do a inline comment to show the point I'm not satisfied

I cannot do any inline comments because it's not close from a change I did:

You can see here I have this resumeProcessing() that can't be called with the current test where done was set in. Doing it breaks the current suite.

We can investigate this in another issue, but for me it looks we are leaking the async functionality somehow and our current tests are returning false positives.

gibson042 · 2016-04-13T20:55:34Z

There seems to be a little cruft from arguments handling, but I'm satisfied overall. LGTM.

Use test.semaphore directly whenever is possible instead of config.current.semaphore

jquerybot added the CLA: Valid label Mar 25, 2016

leobalter added this to the v2.0 milestone Mar 25, 2016

gibson042 reviewed Mar 26, 2016
View reviewed changes

gibson042 mentioned this pull request Mar 26, 2016

Add module-contextual 'test' function #978

Open

leobalter force-pushed the 2.0-start branch 2 times, most recently from f660082 to 74102e4 Compare April 12, 2016 17:50

trentmwillis reviewed Apr 13, 2016
View reviewed changes

All: Remove deprecated features

f549a54

- globals - assertions on QUnit namespace - QUnit.stop - QUnit.init - setup/teardown - asyncTest - test with expected and async parameters - old style for logging callbacks - duration property on testDone details

leobalter force-pushed the 2.0-start branch from 74102e4 to f549a54 Compare April 13, 2016 02:37

jzaefferer reviewed Apr 13, 2016
View reviewed changes

leobalter added 4 commits April 13, 2016 10:32

fixup! Improve error message for removed asyncTest and stop

9756e69

fixup! Throws when start is called in a test context, needs optimization

5bc1982

fixup! rework the deprecated msgs application

70b9d70

fixup! add warnings for module setup and teardown

c5d384a

gibson042 reviewed Apr 13, 2016
View reviewed changes

fixup! rework on tests

97e7e49

leobalter force-pushed the 2.0-start branch from 9eb940b to 97e7e49 Compare April 13, 2016 17:16

fixup! backticks

ef9c175

gibson042 reviewed Apr 13, 2016
View reviewed changes

Test: Refactor test.semaphore usage

21c254a

Use test.semaphore directly whenever is possible instead of config.current.semaphore

Conversation

leobalter commented Mar 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leobalter commented Apr 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trentmwillis commented Apr 13, 2016

Uh oh!

leobalter commented Apr 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leobalter commented Apr 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibson042 commented Apr 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants