Skip to content

Add main site navigation tests for desktop and mobile views#3575

Merged
alexgibson merged 1 commit into
mozilla:masterfrom
davehunt:navigation-tests
Nov 20, 2015
Merged

Add main site navigation tests for desktop and mobile views#3575
alexgibson merged 1 commit into
mozilla:masterfrom
davehunt:navigation-tests

Conversation

@davehunt

Copy link
Copy Markdown
Member

This replaces the following tests from https://github.com/mozilla/mcom-tests:

The main differences are:

  • These tests check the main navigation links from the home page instead of the about and mozilla-based pages. We could easily parameterise these tests if we wanted to run them from multiple initial pages.
  • The tests these replace were checking the links were visible, destination URLs were correct, and destination URLs responded with an OK response code. The new tests click the links (would throw if they were not visible), and check the destination URL is accurate for the expected page.
  • The new tests also check the navigation for small screens by restricting the viewport.

@davehunt

Copy link
Copy Markdown
Member Author

@alexgibson r?

Comment thread tests/pages/base.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason we need to check the height of the menu here? Technically this could change depending on the string/translation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, yes! I'd love to have a better indication that the menu is showing. I can't use is_displayed because then it tries clicking while it's still animating. I could wait for the height to stop changing, but ideally I'd wait for something more concrete. Do you have any suggestions?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking at the code, we add aria-expanded="true" to the _toggle_locator element once the menu opens. Perhaps we could use this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It looks like this attribute is added before the menu is open, as this is failing for me in 2/5 tests.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we can adjust the code here to make testing easier, I'll take a look

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Opened a PR which should hopefully make things easier: #3577

@alexgibson

Copy link
Copy Markdown
Contributor

@davehunt - re: sanity tests, given that these take some time to complete (and one link is also external), perhaps it would be better to have a single santity test that just opens/closes the mobile menu, but doesn't traverse all the links. Thoughts?

@davehunt

Copy link
Copy Markdown
Member Author

re: sanity tests, given that these take some time to complete (and one link is also external), perhaps it would be better to have a single santity test that just opens/closes the mobile menu, but doesn't traverse all the links. Thoughts?

I agree that we should probably avoid depending on an external site for our sanity tests. Do we even need to check the donate menu item given that we're planning to introduce a much more thorough link checker in the near future? I'd suggest dropping the donate check from both tests, and marking just the mobile test as sanity.

@alexgibson

Copy link
Copy Markdown
Contributor

I agree that we should probably avoid depending on an external site for our sanity tests. Do we even need to check the donate menu item given that we're planning to introduce a much more thorough link checker in the near future? I'd suggest dropping the donate check from both tests, and marking just the mobile test as sanity.

Ok, sounds reasonable to me 👍

@davehunt
davehunt force-pushed the navigation-tests branch 4 times, most recently from bba85d0 to bc35334 Compare November 19, 2015 09:51
@davehunt

Copy link
Copy Markdown
Member Author

Rebased and ready for review again. Note that we need #3577 to land before we merge this, but after review I can squash and prepare for the merge.

@alexgibson

Copy link
Copy Markdown
Contributor

LGTM r+ pending #3577

Let's squash & wait for the other PR to merge. Will give these tests a final run once that's in master 👍

@davehunt

Copy link
Copy Markdown
Member Author

Squashed and rebased!

@alexgibson

Copy link
Copy Markdown
Contributor

r+ 🍟

alexgibson added a commit that referenced this pull request Nov 20, 2015
Add main site navigation tests for desktop and mobile views
@alexgibson
alexgibson merged commit df3b6af into mozilla:master Nov 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants