Unify internal testing styles

[thoov]

Currently there are two different styles that tests are written. An older style which uses a QUnit global to define the module and tests via QUnit.module() and QUnit.test(). The old style also uses globals for the assertion such as ok, equal, etc. A newer style which does not use QUnit globals is partially rolled out in some of the packages. This issue tracks the work needed to move all tests over to the newer style and the related work surrounding it.

A breakdown of how to complete the conversion:

1. QUnit.module becomes moduleFor

// Old Style:
QUnit.module('<MODULE_NAME>');

// New Style:
import { moduleFor, AbstractTestCase } from 'internal-test-helpers';

moduleFor('<MODULE_NAME>', class extends AbstractTestCase {
  //...
});

Note: There are a few test case classes already so check here for other options.

2. QUnit.test becomes a property on the test case class

// Old Style:
QUnit.test('<TEST_NAME>', function() {
  // test case here
});

// New Style:
moduleFor('<MODULE_NAME>', class extends TestCase {
  ['@test <TEST_NAME>'](assert) {
    // test case here
  }
});

3. Assertions should be references off of the callback assert argument

// Bad:
QUnit.test('<TEST_NAME>', function() {
  equal(1, 1);
});

// Bad:
moduleFor('<MODULE_NAME>', class extends TestCase {
  ['@test <TEST_NAME>']() {
    this.assert.equal(1, 1);
  }
});

// Good:
moduleFor('<MODULE_NAME>', class extends TestCase {
  ['@test <TEST_NAME>'](assert) {
    assert.equal(1,1)
  }
});

Below are the packages that need to be converted:

• container (🔒 @t-sauer) [Cleanup] Convert container tests to new style #15990
• ember-application (🔒 @locks)
• ember-debug (🔒 @locks) Clean up ember-debug package's tests #15998
• ember-metal (🔒 @thoov) [CLEANUP] Convert ember-metal tests to new style (Part 1) #16034 [CLEANUP] Convert ember-metal accessors tests to new style #16036 [CLEANUP] Convert ember-metal run_loop tests #16051
• ember-routing (🔒 @thoov) [CLEANUP] Convert ember-router tests to new style #16015 [CLEANUP] Convert ember-routing tests to new style (Part 2) #16029 [CLEANUP] Convert ember-routing tests to new style (Part 3) #16030 [CLEANUP] Convert ember-metal/watching tests to new style #16072 [CLEANUP] Convert ember-metal tests over to new style #16105 [CLEANUP] Convert ember-metal/mixin tests over to new style #16106
• ember-runtime (🔒 @thoov)
• ember-template-compiler (🔒 @Karthiick) Cleanup ember-template-compiler's tests #16010
• ember-utils (🔒 @locks) Clean up ember-utils package's tests #15997
• internal-test-helpers (🔒 @thoov) [CLEANUP] Convert internal-test-helpers to new test style #16033
• ember-testing (:lock: @cibernox) Update ember-testing tests to new syntax #16022
• ember (🔒 @locks) Update tests for the ember package #16019 [CLEANUP] Convert ember/tests to new testing style #16449

Below is the test related cleanup that is needed:

• Move off of QUnit global (🔒 @thoov)
• upgrade to QUnit 2 Upgrade to QUnit 2.4.1 (current) 🎉 #16073
• remove testBoth & testWithDefault (🔒 @thoov) [CLEANUP] Removing testBoth and testWithDefault #16450

[rwjblue]

Thanks for putting this together @thoov!

[locks]

I have taken ember-application, ember-debug, and ember-utils: #15986.

[t-sauer]

I will take a stab at container.

[karthiicksiva]

I will take the ember-template-compiler

[mikerhyssmith]

Happy to take a stab at runtime!

[cibernox]

I can take ember-testing

[locks]

Took part of ember: #16019.

[Turbo87]

Can anyone tell me the advantages of the new testing API given that for Ember projects we're now explicitly moving away from moduleFor? 🤔

[rwjblue]

The moduleFor being used here is completely unrelated to the legacy moduleFor implementation that is provided by ember-qunit.

[Turbo87]

@rwjblue I'm aware of that, but it seems that the same pros and cons apply and I'm wondering why this decision was made

[thoov]

@mikerhyssmith For packages/ember-runtime/tests/suites:

It currently has a concept called SuiteBuilder that needs to be converted to the newer AbstractTestCase model.

// BEFORE
const suite = SuiteModuleBuilder.create();

suite.module('includes');

suite.test('includes returns correct value if startAt is positive', function(assert) {
  let data = this.newFixture(3);
  let obj  = this.newObject(data);

  assert.equal(obj.includes(data[1], 1), true, 'should return true if included');
  assert.equal(obj.includes(data[0], 1), false, 'should return false if not included');
});

// AFTER (this class should go in `internal-test-helpers/lib/test-cases)`
class RuntimeTestCase extends AbstractTestCase {
  newFixture() {
    // ...
  }

  newObject() {
    // ...
  }

  // ...
}

// now you can convert the tests pretty much one to one
moduleFor('includes', class extends RuntimeTestCase {
  ['@test includes returns correct value if startAt is positive', function(assert) {
    let data = this.newFixture(3);
    let obj  = this.newObject(data);

    assert.equal(obj.includes(data[1], 1), true, 'should return true if included');
    assert.equal(obj.includes(data[0], 1), false, 'should return false if not included');
  }
});

Let me know if you need any help with this or anything in packages/ember-runtime

[mikerhyssmith]

@thoov thanks ! I noticed a fair few of the remaining tests have testBoth in and some work has been done in the ember-metal tests to support that. Do I need to wait for that to go in first ?

[thoov]

@michaelklishin We are moving away from testBoth as under the hood it is just converting it to this (https://github.com/emberjs/ember.js/blob/master/packages/internal-test-helpers/lib/test-groups.js#L15-L25) however in the second QUnit test it references USES_ACCESSORS which is never set and therefore it just does assert.ok('SKIPPING ACCESSORS'); which isn't useful. Originally we were going to support both cases (which is probably what you saw) but realized that we could just kill that one off. Here is a PR I recently did that moved away from testBoth (https://github.com/emberjs/ember.js/pull/16072/files) that you can use as a guide and let me know if you need help with anything.

[pixelhandler]

@Turbo87 @cibernox @karthiicksiva @locks @mikerhyssmith @rwjblue @t-sauer @thoov is this still an issue, perhaps we should close, what do you think?