Skip to content
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

Auto bind component data-test-* attributes #27

Merged
merged 8 commits into from
Jan 11, 2017
9 changes: 9 additions & 0 deletions addon/.eslintrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module.exports = {
parserOptions: {
ecmaVersion: 6,
sourceType: 'module',
},
env: {
node: true
},
};
16 changes: 16 additions & 0 deletions addon/initializers/ember-test-selectors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import Ember from 'ember';
import bindDataTestAttributes from '../utils/bind-data-test-attributes';

function initialize(/* application */) {
Ember.Component.reopen({
init() {
this._super(...arguments);
bindDataTestAttributes(this);
}
});
}

export default {
name: 'ember-test-selectors',
initialize
};
29 changes: 29 additions & 0 deletions addon/utils/bind-data-test-attributes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import Ember from 'ember';

const TEST_SELECTOR_PREFIX = /data-test-.*/;

export default function bindDataTestAttributes(component) {
let computedBindings = component.attributeBindings && component.attributeBindings.isDescriptor;
if (computedBindings) {
let message = `ember-test-selectors could not bind data-test-* properties on ${component} ` +
`automatically because attributeBindings is a computed property.`;

return Ember.warn(message, false, {
id: 'ember-test-selectors.computed-attribute-bindings',
});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we could somehow make it work correctly in this case as well that would be awesome of course. In theory it should be possible to replace the CP with a wrapper one that we create of course but I'm not sure it's worth the added complexity.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's worth the extra complexity to be honest. I've never seen anyone use a CP for attributeBindings and I have quite a hard time coming up with use cases where that would be a good idea. I'd suggest keeping the warning for now, and if many users report that they are seeing this warning then we can still think about implementing it.

}

let attributeBindings = component.getWithDefault('attributeBindings', []);

if (!Ember.isArray(attributeBindings)) {
attributeBindings = [attributeBindings];
}

for (let attr in component) {
if (TEST_SELECTOR_PREFIX.test(attr)) {
attributeBindings.push(attr);
}
}

component.set('attributeBindings', attributeBindings);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this doesn't work though if the original attributeBindings is a CP?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is true, but I'm not sure if attributeBindings being a CP is supported by Ember at all. otherwise there would be no need for us to apply the bindDataTestAttributes() function before the super constructor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good point. Could you check this though?

that is true, but I'm not sure if attributeBindings being a CP is supported by Ember at all.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm now checking if attributeBindings is a computed property in the bindDataTestAttributes() function and if so, I skip the automatic binding and display a warning message.

}
9 changes: 9 additions & 0 deletions app/.eslintrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module.exports = {
parserOptions: {
ecmaVersion: 6,
sourceType: 'module',
},
env: {
node: true
},
};
1 change: 1 addition & 0 deletions app/initializers/ember-test-selectors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from 'ember-test-selectors/initializers/ember-test-selectors';
28 changes: 26 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
'use strict';

module.exports = {
name: 'test-selectors',
name: 'ember-test-selectors',

_assignOptions: function(app) {
var ui = app.project.ui;
Expand Down Expand Up @@ -37,5 +37,29 @@ module.exports = {
});
}
}
}
},

treeForAddon: function() {
// remove our "addon" folder from the build if we're stripping test selectors
if (!this._stripTestSelectors) {
return this._super.treeForAddon.apply(this, arguments);
}
},

treeForApp: function() {
// remove our "app" folder from the build if we're stripping test selectors
if (!this._stripTestSelectors) {
return this._super.treeForApp.apply(this, arguments);
}
},

preprocessTree: function(type, tree) {
// remove the unit tests if we're testing ourself and are in strip mode.
// we do this because these tests depend on the "addon" and "app" folders being available,
// which is not the case if they are stripped out of the build.
if (type === 'test' && this._stripTestSelectors && this.project.name() === 'ember-test-selectors') {
tree = require('broccoli-stew').rm(tree, 'dummy/tests/unit/**/*.js');
}
return tree;
},
};
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"repository": "https://github.com/simplabs/ember-test-selectors",
"scripts": {
"build": "ember build",
"lint": "eslint config test-support tests *.js",
"lint": "eslint addon app config test-support tests *.js",
"start": "ember server",
"test": "npm run test:keep && npm run test:strip",
"test:all": "ember try:each",
Expand All @@ -26,6 +26,7 @@
},
"devDependencies": {
"broccoli-asset-rev": "^2.4.5",
"broccoli-stew": "^1.4.0",
"ember-ajax": "^2.4.1",
"ember-cli": "2.10.0",
"ember-cli-app-version": "^2.0.0",
Expand Down
51 changes: 51 additions & 0 deletions tests/acceptance/bind-data-test-attributes-in-components-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { test } from 'qunit';
import moduleForAcceptance from '../../tests/helpers/module-for-acceptance';

import config from 'dummy/config/environment';

if (!config.stripTestSelectors) {

/*
* We use an acceptance test here to test the "ember-test-selectors" initializer,
* because initializers are only applied in acceptance tests, but not in
* component integration tests.
*/
moduleForAcceptance('Acceptance | Initializer | ember-test-selectors', {
beforeEach() {
visit('/bind-test');
},
});

test('it binds data-test-* attributes on components', function (assert) {
assert.equal(find('.test1').find('div[data-test-first]').length, 1, 'data-test-first exists');
assert.equal(find('.test1').find('div[data-test-first="foobar"]').length, 1, 'data-test-first has correct value');
});

test('it binds data-test-* attributes on components in block form', function (assert) {
assert.equal(find('.test2').find('div[data-test-first]').length, 1, 'data-test-first exists');
assert.equal(find('.test2').find('div[data-test-first="foobar"]').length, 1, 'data-test-first has correct value');
});

test('it works with multiple data-test-* attributes on components', function (assert) {
assert.equal(find('.test3').find('div[data-test-first]').length, 1, 'data-test-first exists');
assert.equal(find('.test3').find('div[data-test-first="foobar"]').length, 1, 'data-test-first has correct value');
assert.equal(find('.test3').find('div[data-test-second]').length, 1, 'data-test-second exists');
assert.equal(find('.test3').find('div[data-test-second="second"]').length, 1, 'data-test-second has correct value');
});

test('it leaves other data attributes untouched, when a data-test-* attribute is present as well on components', function (assert) {
assert.equal(find('.test4').find('div[data-test-first]').length, 1, 'data-test-first exists');
assert.equal(find('.test4').find('div[data-test-first="foobar"]').length, 1, 'data-test-first has correct value');
assert.equal(find('.test4').find('div[data-non-test]').length, 0, 'data-non-test does not exists');
});

test('it leaves data-test attributes untouched on components', function (assert) {
assert.equal(find('.test5').find('div[data-test]').length, 0, 'data-test does not exists');
});

test('it leaves other data attributes untouched on components', function (assert) {
assert.equal(find('.test6').find('div[data-non-test]').length, 0, 'data-non-test does not exists');
});

}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a test for the case where the component defines attributeBindings (both as a string as well as an array) itself and that asserts that the data-test-* attributes get merged with these correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcoow we test that in the unit tests for the bindDataTestAttributes() function. Can attributeBindings be a string? Unfortunately it's not documented anywhere but I've only found instances where it is used as an array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, just assumed both might actually be supported. If we have a unit test for this already that should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since attributeBindings is a concatenated property, the string will end up in an array eventually. This only works for:

attributeBindings: "myAttribute"

which will become

attributeBindings: ["myAttribute"]

and will not work for:

attributeBindings: "myAttribute myOtherAttribute"

which would result in attributeBindings: ["myAttribute myOtherAttribute"]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://emberjs.com/api/classes/Ember.View.html#toc_html-attributes only uses arrays, so I'm assuming passing a string is not officially supported.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's not official API, I would tend to agree with @Turbo87 and not support this tbh.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented a quick array wrapper now, see f48e6ad


1 change: 1 addition & 0 deletions tests/dummy/app/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const Router = Ember.Router.extend({
});

Router.map(function() {
this.route('bind-test');
});

export default Router;
11 changes: 11 additions & 0 deletions tests/dummy/app/templates/bind-test.hbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<div class="test1">{{data-test-component data-test-first="foobar"}}</div>

<div class="test2">{{#data-test-component data-test-first="foobar"}}hello{{/data-test-component}}</div>

<div class="test3">{{data-test-component data-test-first="foobar" data-test-second="second"}}</div>

<div class="test4">{{data-test-component data-test-first="foobar" data-non-test="baz"}}</div>

<div class="test5">{{data-test-component data-test="foo"}}</div>

<div class="test6">{{data-test-component data-non-test="foo"}}</div>
120 changes: 120 additions & 0 deletions tests/unit/utils/bind-data-test-attributes-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
import { module, test } from 'qunit';
import Ember from 'ember';

import bindDataTestAttributes from 'ember-test-selectors/utils/bind-data-test-attributes';

module('Unit | Utility | bind data test attributes');

test('it adds missing attributeBindings array', function(assert) {
let Fixture = Ember.Object.extend({
'data-test-from-factory': 'foo',
});
let instance = Fixture.create({
'data-test-from-invocation': 'bar',
});

assert.deepEqual(instance.get('attributeBindings'), undefined);

bindDataTestAttributes(instance);

assert.deepEqual(instance.get('attributeBindings'),
['data-test-from-invocation', 'data-test-from-factory']);
});

test('it adds to existing attributeBindings array', function(assert) {
let Fixture = Ember.Object.extend({
attributeBindings: ['foo', 'bar'],

foo: 1,
bar: 2,

'data-test-from-factory': 'foo',
});
let instance = Fixture.create({
'data-test-from-invocation': 'bar',
});

assert.deepEqual(instance.get('attributeBindings'), ['foo', 'bar']);

bindDataTestAttributes(instance);

assert.deepEqual(instance.get('attributeBindings'),
['foo', 'bar', 'data-test-from-invocation', 'data-test-from-factory']);
});

test('it converts existing attributeBindings string to array', function(assert) {
let Fixture = Ember.Object.extend({
attributeBindings: 'foo',

foo: 1,

'data-test-from-factory': 'foo',
});
let instance = Fixture.create({
'data-test-from-invocation': 'bar',
});

assert.deepEqual(instance.get('attributeBindings'), 'foo');

bindDataTestAttributes(instance);

assert.deepEqual(instance.get('attributeBindings'),
['foo', 'data-test-from-invocation', 'data-test-from-factory']);
});

test('it only adds data-test-* properties', function(assert) {
let Fixture = Ember.Object.extend({
foo: 1,
bar: 2,

'data-test-from-factory': 'foo',
});
let instance = Fixture.create({
baz: 3,

'data-test-from-invocation': 'bar',
});

assert.deepEqual(instance.get('attributeBindings'), undefined);

bindDataTestAttributes(instance);

assert.deepEqual(instance.get('attributeBindings'),
['data-test-from-invocation', 'data-test-from-factory']);
});

test('it does not add a data-test property', function(assert) {
let Fixture = Ember.Object.extend({
'data-test': 'foo',
});
let instance = Fixture.create();

assert.deepEqual(instance.get('attributeBindings'), undefined);

bindDataTestAttributes(instance);

assert.deepEqual(instance.get('attributeBindings'), []);
});

test('it skips if attributeBindings is a computed property', function(assert) {
let Fixture = Ember.Object.extend({
attributeBindings: Ember.computed('prop', function() {
return [this.get('prop')];
}).readOnly(),

foo: 5,

'data-test-from-factory': 'foo',
});
let instance = Fixture.create({
prop: 'foo',

'data-test-from-invocation': 'bar',
});

assert.deepEqual(instance.get('attributeBindings'), ['foo']);

bindDataTestAttributes(instance);

assert.deepEqual(instance.get('attributeBindings'), ['foo']);
});