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

Update busy model wrapper component to use page object #1625

Merged

Conversation

daveconnis
Copy link
Contributor

What's in this PR?

Adding page object to busy-model-wrapper-test and writing out tests that actually test the component. Originally, there was only an it renders test. Planning on testing both onSaving and onDeleting.

Fixes #1607

@daveconnis
Copy link
Contributor Author

Unsure what I need in the page object because I'm not sure what model is in /Users/imaginationstation/sites/code-corps-ember/app/templates/components/common/busy-model-wrapper.hbs. What is this referencing? It isn't the model which is set to null in the corresponding .js file is it? if so, where is the action defined for setting model to model.isSaving or model.isDeleting

@begedin
Copy link
Contributor

begedin commented Dec 18, 2017

@daveconnis The idea of that component, is, you give it an ember data model and wrap the component around some sort of UI related to that model. If the model is not being saved or deleted at the moment, it shows that UI. Otherwise, it shows a message indicating it's saving or deleting.

From that, you don't really need to put anything in the page object, although we may at some point

What you want to test is

  • if model is saving, but is not deleted, page.text is "Saving"
  • if model is saving and is deleted, page.text is "Deleting"
  • if model is neither saving nor deleted, page.text is whatever you wrapped the component around

If you set your render page as

page.render(hbs`{{#budy-model-wrapper model=model}}Foo{{/busy-model-wrapper}}`)

Then bullet nr. 3 simply asserts page.text is 'Foo'

@begedin
Copy link
Contributor

begedin commented Dec 18, 2017

So to add to my previous comment, your direction is fine. Just need to cover the 2 other cases.


assert.equal(this.$().text().trim(), 'template block text');
assert.equal(page.text, 'Foo', 'the model returns block');
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have the first two tests passing, by setting the model, however, I'm unsure how to get the this one passing without something to yield. Nikola mentioned earlier that I could set my renderPage to page.render(hbs{{#busy-model-wrapper model=model}}Foo{{/busy-model-wrapper}}) but I've tried this both in my initial renderPage() function and in the test itself and haven't gotten anywhere with it. The error I get is

Error: Element not found.

PageObject: 'page.text'
Selector: ':first'

I got this on both of my tests prior, but only because I'd messed up the boolean values onSaving and onDeleting. I fixed that, but I'm not sure what this would need to accept that neither onSaving or onDeleting are false.

Copy link
Contributor

@joshsmith joshsmith Dec 19, 2017

Choose a reason for hiding this comment

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

You won't want to use renderPage for this since that's just a helper function.

In this case you would do:

page.render(hbs`
  {{#common/busy-model-wrapper model=model}}
    Foo
  {{/common/busy-model-wrapper}}
`);

inside of your test.

@daveconnis
Copy link
Contributor Author

I've deleted the tag, class, and page object for this, haven't switched the page object syntax back to jQuery yet, but when I do, I'll be sure to comment out WHY we haven't switched this over to a page object.

@joshsmith
Copy link
Contributor

@daveconnis do you want to finish this one up before continuing on the other?

let model = { isSaving: true, isDeleted: false };
set(this, 'model', model);
renderPage();
// These tests do not make use of a page object. One was written, but it consisted of an empty string and a .busymodelwrapper scope. Set up this way, the page object wasn't actually rendering anything, just empty code placeholders. Switched to using jQuery selectors in order to have cleaner more effectived code.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Biggest thing I want attention on is this comment. Want to make sure it makes sense. Remember, we veto'd the page object for this test because it was just, if I remember right:
tagname: ' ' scope: .busymodelwrapper


moduleForComponent('common/busy-model-wrapper', 'Integration | Component | common/busy model wrapper', {
integration: true
});

test('it renders', function(assert) {
// These tests do not make use of a page object. One was written, but it consisted of an empty string and a .busymodelwrapper scope. Set up this way, the page object wasn't actually rendering anything, just empty code placeholders. Switched to using jQuery selectors in order to have cleaner more effectived code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments should be multi-line when over 80 chars.

@daveconnis daveconnis force-pushed the update-busy-model-wrapper-component-to-use-page-object branch from c521076 to 3e0fcdf Compare January 8, 2018 19:34
@begedin begedin force-pushed the update-busy-model-wrapper-component-to-use-page-object branch from 3e0fcdf to c69b6e9 Compare January 19, 2018 14:50
@begedin
Copy link
Contributor

begedin commented Jan 19, 2018

@daveconnis I decided to wrap this up.

In hindsight, we didn't pay enough attention on how the busy model wrapper works and sent you in the wrong direction.

What I ended up doing is I used your findings to setup a dynamically defined page object and a render-page function which wraps the component call in a test container div, so we can still assert page.text instead of this.$().text().trim()

@begedin begedin merged commit c8c8ede into develop Jan 19, 2018
@begedin begedin deleted the update-busy-model-wrapper-component-to-use-page-object branch January 19, 2018 15:00
jderr-mx pushed a commit to jderr-mx/code-corps-ember that referenced this pull request Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite integration tests for busy-model-wrapper-test component
3 participants