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

[tests] Cleanup tests #486

Merged
merged 1 commit into from
Feb 26, 2018
Merged

[tests] Cleanup tests #486

merged 1 commit into from
Feb 26, 2018

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Feb 7, 2018

This PR cleans up the tests by utilizing page objects and breaking
out tests into various files. It also attempts to start formalizing
some scenario helpers that can be used to run the same modules against
multiple scenarios.

@pzuraq
Copy link
Contributor Author

pzuraq commented Feb 26, 2018

@Addepar/ice

@@ -10,6 +13,14 @@ export default class CustomFooter extends Component {
@argument rowIndex;
@argument onFooterEvent;

@computed('column.valuePath', 'rowValue')
get footerValue() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need this footerValue for this custom component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed for tests, but the template renders it and it wasn't there. I'd assumed it was meant to be and added it for future tests, because we'll likely want to verify that it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a duplication of footerValue in ember-table-footer. I think I put it there in the template but did not actually use it. We can pass footerValue=footerValue in ember-table-footer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should do that just to reduce code. Going to merge this, we need to keep moving.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's good to me. We can clean up this later. I did not mean to bikeshed on this.

assert.ok(table.rows.length < rowCount, 'not all rows have been rendered');
assert.equal(table.getCell(0, 0).text.trim(), '0A', 'correct first row rendered');
assert.notEqual(
table.getCell(table.rows.length - 1, 0).text.trim(), '99A', 'correct last row rendered'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the message here does not reflect what it test. It should say "Last rendered row is not last data row"

// scroll all the way down
await scrollTo('[data-test-body-container]', 0, 10000);

assert.notEqual(table.getCell(0, 0).text.trim(), '0A', 'correct first row rendered');
Copy link
Contributor

Choose a reason for hiding this comment

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

same

This PR cleans up the tests by utilizing page objects and breaking
out tests into various files. It also attempts to start formalizing
some scenario helpers that can be used to run the same modules against
multiple scenarios.
@pzuraq pzuraq merged commit 2195e74 into master Feb 26, 2018
@pzuraq pzuraq deleted the cleanup/tests branch February 26, 2018 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants