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

Try fixing CTA test. #3057

Merged
merged 6 commits into from
Aug 21, 2019
Merged

Try fixing CTA test. #3057

merged 6 commits into from
Aug 21, 2019

Conversation

miina
Copy link
Contributor

@miina miina commented Aug 20, 2019

Improves the CTA test by explicitly selecting the Page block before Removing.

@googlebot googlebot added the cla: yes Signed the Google CLA label Aug 20, 2019
@miina miina marked this pull request as ready for review August 20, 2019 22:19
@miina
Copy link
Contributor Author

miina commented Aug 20, 2019

Ran the e2e tests 3 times, passed every time.

@miina miina requested a review from swissspidy August 20, 2019 22:29
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Can you remove the eslint-disable comment too? :-)

@miina
Copy link
Contributor Author

miina commented Aug 20, 2019

Done. Maybe it will fail now :D

@miina
Copy link
Contributor Author

miina commented Aug 20, 2019

And yes, it did fail now. Will look into it tomorrow again then.

@miina
Copy link
Contributor Author

miina commented Aug 21, 2019

OK, now it passed 5 times in a row.

Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

I don't see how the switchEditorModeTo addition is relevant here, but it looks reasonable.

@swissspidy swissspidy merged commit ccf56ec into develop Aug 21, 2019
@swissspidy swissspidy deleted the fix/cta-test branch August 21, 2019 14:23
@swissspidy swissspidy added Infrastructure Changes impacting testing infrastructure or build tooling AMP Stories Bug Something isn't working labels Aug 21, 2019
@swissspidy swissspidy added this to the v1.2.2 milestone Aug 21, 2019
@miina
Copy link
Contributor Author

miina commented Aug 21, 2019

I don't see how the switchEditorModeTo addition is relevant here, but it looks reasonable.

A few times when Travis failed, the code-editor.js test failed and not the CTA test, apparently due to switchEditorModeTo using click not working reliably.

@westonruter
Copy link
Member

Should we not milestone this for v1.2.1 instead of v1.2.2?

@miina miina modified the milestones: v1.2.2, v1.2.1 Aug 21, 2019
@miina
Copy link
Contributor Author

miina commented Aug 21, 2019

Yep! Changed.

westonruter added a commit that referenced this pull request Aug 22, 2019
* tag '1.2.1': (434 commits)
  Bump 1.2.1
  Adjust block inserter style. (#3075)
  Update dependency eslint-plugin-jest to v22.15.2
  Fix flaky CTA test (#3057)
  Add more resizing handles (#3023)
  Bump version to 1.2.1-RC1
  Improve story page background media with image srcset, reduced image size, and a11y text (#3006)
  Wait until content loaded before calculating font size. (#3052)
  Harden logic for normalizing image metadata before adding story images (#3049)
  Update dependency uuid to v3.3.3 (#3046)
  Inline color support (#3033)
  Update dependency webpack-cli to v3.3.7 (#3040)
  Tiny prop-types fix
  Update dependency babel-jest to v24.9.0 (#3037)
  Update e2e test setup (#3031)
  Include amp-script among dynamic_element_selectors in tree shaking
  Another try to fix tests.
  Ensure selecting the first page before removing the block.
  Move setting input selection to the end to helpers.
  Fix editor test.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working cla: yes Signed the Google CLA Infrastructure Changes impacting testing infrastructure or build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants