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.

Sign up for GitHub

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

Jump to bottom

Adding integration tests for nodes page #1707

Merged
merged 3 commits into from
Jan 13, 2017
Merged

Adding integration tests for nodes page #1707

merged 3 commits into from
Jan 13, 2017

Conversation

mlunoe
Copy link
Contributor

@mlunoe mlunoe commented Dec 27, 2016

@d2iq-mergebot
Copy link

This repo has @mesosphere-mergebot integration. You can interact with the following commands.

@mesosphere-mergebot integrate-it
@mesosphere-mergebot ship-it
@mesosphere-mergebot test-it

@mlunoe mlunoe assigned MatApple and unassigned jfurrow Dec 27, 2016
@mlunoe
Copy link
Contributor Author

mlunoe commented Jan 6, 2017

ping @Poltergeist

@mlunoe mlunoe assigned rcorral and unassigned MatApple Jan 6, 2017
@jfurrow
Copy link
Contributor

jfurrow commented Jan 6, 2017

Works well! 🚢

@jfurrow jfurrow self-assigned this Jan 6, 2017
@mlunoe mlunoe assigned nLight and unassigned Poltergeist Jan 10, 2017
@nLight
Copy link
Contributor

nLight commented Jan 11, 2017

Unfortunately I can't run the integration tests to verify the PR

@nLight nLight removed their assignment Jan 11, 2017
@mlunoe mlunoe force-pushed the mlunoe/tests/DCOS-6596-add-integration-tests-for-nodes-page branch from b95f614 to 84cb114 Compare January 12, 2017 23:12
@mlunoe
Copy link
Contributor Author

mlunoe commented Jan 12, 2017

Rebased with integration test fixes

@mlunoe
Copy link
Contributor Author

mlunoe commented Jan 12, 2017

Run unit tests

2 similar comments
@mlunoe
Copy link
Contributor Author

mlunoe commented Jan 13, 2017

Run unit tests

@mlunoe
Copy link
Contributor Author

mlunoe commented Jan 13, 2017

Run unit tests

@rcorral rcorral force-pushed the mlunoe/tests/DCOS-6596-add-integration-tests-for-nodes-page branch from 84cb114 to bee8e64 Compare January 13, 2017 17:22
@mlunoe
Copy link
Contributor Author

mlunoe commented Jan 13, 2017

rcorral
rcorral reviewed Jan 13, 2017
View reviewed changes
Copy link
Contributor

@rcorral rcorral left a comment

Choose a reason for hiding this comment

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

Just wanted to show some improvements I made to the tests you wrote and why I made the changes.

tests/pages/nodes/NodesPage-cy.js
});
cy.visitUrl({url: '/nodes'});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Moved this code under the Filters nodes table context because in the Nodes grid context we are doing a different call to visitUrl we should do a best-effort to only call visitUrl once per test as this is expensive and time consuming and can easily cause each test to take an extra couple seconds to run.

tests/pages/nodes/NodesPage-cy.js
cy.get('@hostnames')
.should('contain', 'dcos-01')
.should('contain', '167.114.218.155')
.should('contain', '167.114.218.156');
Copy link
Contributor

Choose a reason for hiding this comment

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

A more efficient way of running these assertions. All of the should calls run against the last filter call, in this case the call to get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right on!

tests/pages/nodes/NodesPage-cy.js
cy.get('@filterBar').contains('Filter by Service').click();
cy.get('.dropdown-menu').contains('cassandra-unhealthy').click();
cy.get('@filterBar').contains('Healthy').click();
context('Filters nodes grid', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting the rest of the tests for the Nodes grid in their own context so that we don't call visitUrl more than once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! Yeah, I guess this isn't an actual issue, but just more efficient, right?

tests/pages/nodes/NodesPage-cy.js
cy.configureCluster({
mesos: '1-for-each-health',
nodeHealth: true
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this down here now, because we moved the beforeEach from the code above into the context above.

@rcorral
Copy link
Contributor

rcorral commented Jan 13, 2017 •
edited
Loading

run integration tests

damn EOF

@rcorral rcorral merged commit 2a3baea into master Jan 13, 2017
@rcorral rcorral deleted the mlunoe/tests/DCOS-6596-add-integration-tests-for-nodes-page branch January 13, 2017 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcorral rcorral rcorral left review comments

Assignees

@rcorral rcorral

@jfurrow jfurrow

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mlunoe @d2iq-mergebot @jfurrow @nLight @rcorral @Poltergeist @MatApple