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

Additional locator acceptance tests #830

Merged
merged 23 commits into from
Jun 15, 2021
Merged

Conversation

cea2aj
Copy link
Member

@cea2aj cea2aj commented Jun 11, 2021

Add additional locator acceptance tests

These tests cover:

  • Pagination
  • Default initial search
  • Interacting with the results list

This PR also includes some additional updates

  • Set yxt-Pin and yxt-PinCluster CSS classes so that I can safely target them from the tests. This should only affect Mapbox since google does not use HTML pins
  • Updated the functions on the blocks to all be async. I think this consistency makes it easier to write acceptance tests. It also separates concerns by keeping the lower-level Selector object within the blocks.

J=SLAP-1362
TEST=acceptance

@cea2aj cea2aj changed the base branch from master to develop June 11, 2021 14:47
@coveralls
Copy link

coveralls commented Jun 11, 2021

Coverage Status

Coverage increased (+0.04%) to 5.562% when pulling e115cb6 on dev/additional-locator-acceptance into 3778264 on develop.

@tmeyer2115
Copy link
Collaborator

Add additional locator acceptance tests

J=SLAP-1362
TEST=acceptance

Can you provide high level descriptions of the tests added?

@cea2aj
Copy link
Member Author

cea2aj commented Jun 11, 2021

Can you provide high level descriptions of the tests added?

sure

tests/acceptance/blocks/page.js Show resolved Hide resolved
@@ -5,5 +5,6 @@ GITHUB_BRANCH=${GITHUB_REF#refs/heads/}
export BROWSERSTACK_BUILD_ID="${GITHUB_BRANCH} - ${GITHUB_RUN_ID}"
COMMIT_MSG_TITLE=$(git log -n 1 --pretty=format:%s)
export BROWSERSTACK_TEST_RUN_NAME=$COMMIT_MSG_TITLE
export IS_DEVELOPMENT_PREVIEW="false"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we ever figure out why the tests were running in dev mode? By default, it should be Prod

Copy link
Member Author

Choose a reason for hiding this comment

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

I mostly added this here for clarity. Why did we believe they were running in dev mode again? When I look at the acceptance tests for the PR where I removed postcss, they successfully ran in IE11 in browserstack which isn't possible if it were running in dev mode

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's possible there was a branch mismatch or something. The reason Oshi and I believed they were running in dev mode is because the tests passed when postcss was removed. The InlineAssetHTMLPlugin requires postcss to function. So, if it were removed and Prod mode was used, the grunt webpack command should have failed. Maybe you were branched off something older where InlineAssetHTMLPlugin did not have that dependency.

tests/acceptance/blocks/thememap.js Outdated Show resolved Hide resolved
tests/acceptance/blocks/verticalresults.js Outdated Show resolved Hide resolved
@@ -5,5 +5,6 @@ GITHUB_BRANCH=${GITHUB_REF#refs/heads/}
export BROWSERSTACK_BUILD_ID="${GITHUB_BRANCH} - ${GITHUB_RUN_ID}"
COMMIT_MSG_TITLE=$(git log -n 1 --pretty=format:%s)
export BROWSERSTACK_TEST_RUN_NAME=$COMMIT_MSG_TITLE
export IS_DEVELOPMENT_PREVIEW="false"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's possible there was a branch mismatch or something. The reason Oshi and I believed they were running in dev mode is because the tests passed when postcss was removed. The InlineAssetHTMLPlugin requires postcss to function. So, if it were removed and Prod mode was used, the grunt webpack command should have failed. Maybe you were branched off something older where InlineAssetHTMLPlugin did not have that dependency.

*/
async selectMapboxPin () {
async selectPin () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this and selectPinCluster is there any way to target a specific pin or cluster? Just curious. Doesn't seem like we need that for this PR, but we may want that for future tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it should be possible to select specific pins or clusters since the different clusters and pins have different aria labels that we can use for targeting.

tests/acceptance/blocks/verticalresults.js Outdated Show resolved Hide resolved
tests/acceptance/blocks/page.js Show resolved Hide resolved
@@ -14,6 +15,7 @@ class Pagination {
*/
async nextResults () {
await Page.scrollToBottom(); // We must scroll to the bottom of the page to be able to click the next results page button
Copy link
Collaborator

Choose a reason for hiding this comment

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

For my own edification, we first need to scroll to the bottom of the page, then scroll to the bottom of the VR to click the button?

Copy link
Member Author

@cea2aj cea2aj Jun 14, 2021

Choose a reason for hiding this comment

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

Correct. It's needed primarily for screens with smaller heights. I'm actually making an item for this because when the height of the browser is small, the vertical results scroll bar can get hidden, and scrolling the page is the only way to get to the bottom of the vertical results

@cea2aj cea2aj requested review from tmeyer2115 and oshi97 June 14, 2021 21:36
Copy link
Contributor

@oshi97 oshi97 left a comment

Choose a reason for hiding this comment

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

lgtm!

@cea2aj cea2aj merged commit 5356124 into develop Jun 15, 2021
@cea2aj cea2aj deleted the dev/additional-locator-acceptance branch June 15, 2021 14:58
@cea2aj cea2aj mentioned this pull request Jun 23, 2021
cea2aj added a commit that referenced this pull request Jun 23, 2021
## Version 1.22.0
### Changes
- Code coverage reports are now generated automatically for the Theme. (#759)
- A number of dependencies were updated in accordance with Dependabot's suggestions. (#780)
- The build time in `Development` mode was reduced significantly. (#782, #829) Build times in 
`Production` mode were also decreased. (#783, #825)
- Two new cards supporting videos were added: `product-prominentvideo` and `multilang-product-prominentvideo`. These cards
work with either YouTube or Vimeo content. (#794)
- The process for formatting hours was made more efficient, resulting in faster renders for cards with hours. (#784)
- Any SDK branch can now be specified in the `global_config.sdkVersion` attribute. (#791)
- Unused CSS was removed from the Theme. (#793, #798)
- To reduce flashing related to the SDK Component lifecycle, placeholder skeletons were added for the `SearchBar` and
`Navigation` components. (#797)
- Additional acceptance tests were added for the `VerticalFullPageMap` and Percy snapshots are now generated for all
cards. (#830, #820)
- Multi-language tests were added for the Theme. (#826)
- The `documentsearch-standard` card now supports rich text featured snippets. (#838)
- A new concept, `RuntimeConfig` was added. This allows configuration, computed at run-time, to be injected into the
Theme and SDK. This new concept enables JWT integrations as well. (#843)
### Bug Fixes
- A number of changes were made to make the Theme's visual regression tests more reliable. (#764, #765, #766, #778)
- Corrected how the CTA URLs for the `menuitem-standard` and `multilang-menuitem-standard` cards are computed. (#787)
- The Google Streetview person icon is now visible when using the `VerticalFullPageMap` layout. (#801)
- The size of the cluster and result pins are now configurable. (#807)
- A double encoding error for `verticalUrl` was fixed. (#836)
- The `Opens At` open status message is now properly translated in all languages. (#834)
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.

4 participants