-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Acceptance / Feature tests #4190
Conversation
It's not obvious what "the way" to teardown window event handlers is in Ember. The datacenter-picker is permanently in the app during usage, but in tests I'm assuming it gets added and removed lots. So when you run the tests, as the tests aren't run in an isolated runner the QUnit test runner ends up with a click handler on it, So if you click on the test runner one of the tests will fail. The failure is related to there not being an element with a `.contains` method. So this checks that the element is truthy first, i.e. it exists. If it doesn't it just bails out.
Go with `rsync` over `cp` for putting the api double into public for the moment
`rect.top` is zero until the tab panel becomes visible, resize will need to be called when the tab is clicked also
Tables need to calculate their sizing depending on other things in the DOM. When a table is in a tab panel, some of these things aren't visible and therefore some values are zero during `didInsertElement`. This commit ensures that the resize calc of the table is performed when it's parent tab is clicked (and therefore when the table 'appears')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These gherkin tests look pretty nice. It's hard to tell exactly what is being tested when the tests are so far abstracted from the application, but all the tests passed and it seems like pretty good coverage.
I had a bunch of questions and comments that I left inline.
if (app.env === 'production') { | ||
tree = stew.rm(tree, 'consul-api-double'); | ||
} | ||
return tree; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to remove the api double assets in the public
dir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, long story.. I was trying to use broccoli
to sanely make the npm
module of consul-api-double
available in /
only when testing. Gave up in the end then thought differently and figured I could also add it in all the time and instead delete it for production. This is a temporary solution for the moment (related to the rsync
stuff down below), I asked about a while ago to see if anyone could help with this but no-go, let me know if you think you could lend a hand.
"precommit": "lint-staged" | ||
"test:view": "ember test --server", | ||
"precommit": "lint-staged", | ||
"postinstall": "rsync -aq ./node_modules/@hashicorp/consul-api-double/ ./public/consul-api-double/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type of file shuffling could be handled by an ember-addon. Then it would happen behind the scenes without having to rely on these npm hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely, exactly what I'd like to know how to do properly, or better still just get ember to serve the folder in /
without copying it. Something that can be done in development
but not in test
it seems :(, see previous comment also. Let me know if you know how I can do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This area of ember is not my forte, but I'm definitely down to help out.
"devDependencies": { | ||
"@hashicorp/consul-api-double": "^1.0.0", | ||
"@hashicorp/ember-cli-api-double": "^1.0.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between these two packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consul-api-double
is the consul specific representation of the consul API and ember-cli-api-double
is an ember-addon wrapper around a framework independent 'renderer' for 'api doubles'.
The thinking here is you could theoretically have a really simple to write vault-api-double
, nomad-api-double
etc etc, and if you wanted to use it in ember
you could. Additionally, as its entirely framework independent, anyone else could also use it in Vue, Choo, React, Polymer, Angular or anything else to help them to test their UI's (so hashi ui, cui etc etc)
@@ -40,6 +46,7 @@ | |||
"ember-cli-app-version": "^3.0.0", | |||
"ember-cli-autoprefixer": "^0.8.1", | |||
"ember-cli-babel": "^6.6.0", | |||
"ember-cli-cjs-transform": "^1.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm not sure, think this can go now actually, it's probably left over from when I was testing out broccolli
bits, will check and see if its removable, good spot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, you defo need it, just tried it out. It's weird though because it's actually needed by a dependency, I remember trying to get it to work by specifying it in the dependency instead, but as far as I remember that didn't work either.
I've put this in my backlog to relook at it at a later date
Feature: Acl Filter | ||
Scenario: Filtering [Model] | ||
Given 1 datacenter model with the value "dc-1" | ||
And 2 [Model] models |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there value in making Model a variable rather than inlining acl
? acl
is the only Model value here, and the file is called acl-filter
. The variable seems like unnecessary indirection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably yeah, but it remains clear enough (IMO). I think I was looking to see if I could include it in the catalog-filter
tests so I could just test all the filters in on area. Will probably still look to do that further down the line.
url, | ||
data | ||
) { | ||
const request = api.server.history[api.server.history.length - 2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks brittle. Is it guaranteed that the request in question will always be the second-to-last one made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hehe agree completely here, this is on my todo list. This is specific to my needs right now, and isn't 'brittle' as such, just not as I would like it. It's a sufficient test for what I need it for right now though.
ui-v2/tests/steps.js
Outdated
assert.equal( | ||
request.method, | ||
method, | ||
`Expected the request method to be ${method} but was ${request.method}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text gets displayed no matter what, which makes for a weird reading experience when the test passes:
Expected the request method to be PUT but was PUT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh, cool, will change 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, you only really see these when they fail, unless you specifically click to show passed tests, and then open up the accordion for the test. I've tweaked the text though to make it less weird, plus entered the competition for best commit message 😄
ui-v2/tests/steps.js
Outdated
body, | ||
data, | ||
`Expected the request body to be ${body} but was ${request.requestBody}` | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion can fail if the two bodies are ordered differently but functionally equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool, yeah good spot, deepEqual
or something would be better here I suppose. Can sort that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realized deepEqual
won't work for me here, I want to be able to match against a partial object (so I might not want to specify the entire payload in my test fixture). I think you've made a good point though, if/when it fails because of the wrong order I'll be aware and can sort it then. For testing purposes it doesn't matter that the properties in the payload are in the right or wrong order so what it does at the moment is fine for me right now.
assert.ok(true); | ||
}) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's neat how well this all plays with ember-cli-page-object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Yeah, in my past life the idea of page objects for acceptance were an essential building block.
Did you see it also plays well with mirage if you want it to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, reading over the bits for creating models and asserting against Pretender looks like it could easily be adapted to work with Mirage.
}, | ||
}); | ||
assert.equal(actual, expected); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests look fine, but the concept of a getComponentFactory
is an anti-pattern. You shouldn't be looking up views and invoking things on them from controllers. That's the antithesis of data-down actions-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is a bit of a bigger one. I wouldn't necessarily say this is an 'anti-pattern'. I had a good think about this before doing it this way, and I read plenty of opinions online by ember-y people who also thought that DDAU wasn't the ideal way to deal with this either. I'd possibly equate it to saying that you should use DDAU for window resizing instead of listening to the resize event, which probably isn't a good plan.
Other options I read about we concerned with making a global message bus, or registering the components with the parent controller via attributes, both of which weren't decoupled enough for me. I didn't want to add anything special to the component in order to achieve what I needed.
What I've done is pretty much how standard web-components work, well.. as close to it as I could get. Happy to discuss this more if you feel it is absolutely the wrong thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I am misinterpreting what the util is used for then. The place I saw it being used, it looked like you could instead use a didRender
hook or an observer (also considered an anti-pattern, but sometimes unavoidable and better than looking up views in the registry).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff thanks @DingoEatingFuzz, much appreciated ! Some good spots in there, overall please take into account this is currently still WIP, and likely to be refined much more once my current tasks are complete. If you want chat more about ins and outs you know where I am 👍 . Would love to get some more hands on this.
WIP: Ideally all of these would go
1. All {{ivy-codemirror}} components need 'refreshing' when they become visible via our own `didAppear` method on the `{{code-editor}}` component (also see:) - #4190 (comment) - https://github.com/hashicorp/consul/pull/4789/files/73db111db8324e1c6710791b38ba1cd2bab08f15#r225264296 2. On initial investigation, it looks like the component we are using for the code editor doesn't distinguish between setting its `value` programatically and a `keyup` event, i.e. an interaction from the user. We currently pretend that whenever its `value` changes, it is a `keyup` event. This means that when we reset the `value` to `""` programmatically for form resetting purposes, a 'pretend keyup' event would also be fired, which would in turn kick off the validation, which would fail and show an error message for empty values in other fields of the form - something that is perfectly valid if you haven't typed anything yet. We solved this by checking for `isPristine` on fields that are allowed to be empty before you have typed anything.
1. All {{ivy-codemirror}} components need 'refreshing' when they become visible via our own `didAppear` method on the `{{code-editor}}` component (also see:) - #4190 (comment) - https://github.com/hashicorp/consul/pull/4789/files/73db111db8324e1c6710791b38ba1cd2bab08f15#r225264296 2. On initial investigation, it looks like the component we are using for the code editor doesn't distinguish between setting its `value` programatically and a `keyup` event, i.e. an interaction from the user. We currently pretend that whenever its `value` changes, it is a `keyup` event. This means that when we reset the `value` to `""` programmatically for form resetting purposes, a 'pretend keyup' event would also be fired, which would in turn kick off the validation, which would fail and show an error message for empty values in other fields of the form - something that is perfectly valid if you haven't typed anything yet. We solved this by checking for `isPristine` on fields that are allowed to be empty before you have typed anything.
1. All {{ivy-codemirror}} components need 'refreshing' when they become visible via our own `didAppear` method on the `{{code-editor}}` component (also see:) - #4190 (comment) - https://github.com/hashicorp/consul/pull/4789/files/73db111db8324e1c6710791b38ba1cd2bab08f15#r225264296 2. On initial investigation, it looks like the component we are using for the code editor doesn't distinguish between setting its `value` programatically and a `keyup` event, i.e. an interaction from the user. We currently pretend that whenever its `value` changes, it is a `keyup` event. This means that when we reset the `value` to `""` programmatically for form resetting purposes, a 'pretend keyup' event would also be fired, which would in turn kick off the validation, which would fail and show an error message for empty values in other fields of the form - something that is perfectly valid if you haven't typed anything yet. We solved this by checking for `isPristine` on fields that are allowed to be empty before you have typed anything.
WIP Acceptance tests using Gherkin and consul-api-double.
I'm not sure what people are aware of here so for more info, please ask, happy to walk people through.
There are a couple of additional 'features' included in here:
There are a couple of things in here I would like some help with (mainly broccoli related), again please ask and I can walk you through.