Skip to content

Conversation

@sergeymitr
Copy link
Contributor

Rebuilding the connection package unit test Test_REST_Endpoints::test_remote_authorize().

Changes proposed in this Pull Request:

The existing test only verifies the existence of the endpoint, and turned out to be unreliable.
The test has been rewritten to actually test the authorization process. Some existing tests were also refactored.

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Run unit tests for the jetpack-connection package.
Make sure that all tests pass.

Proposed changelog entry for your changes:

n/a

The existing test only verifies the existence of the endpoint, and turned out to be unreliable.
The test has been rewritten to actually test the authorization process. Some existing tests were also refactored.
@sergeymitr sergeymitr added [Status] Needs Review This PR is ready for review. Unit Tests Connect Flow Connection banners, buttons, ... labels Aug 18, 2020
@sergeymitr sergeymitr requested a review from kbrown9 August 18, 2020 16:23
@sergeymitr sergeymitr self-assigned this Aug 18, 2020
Copy link

@test-case-reminder test-case-reminder bot left a comment

Choose a reason for hiding this comment

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

Here are some suggested test cases for this PR.

Connection

  • In-place connection with free plan
  • In-place connection with paid plan
  • In-place connection with product purchase
  • Classic connection. Use Safari, or set a constant JETPACK_SHOULD_NOT_USE_CONNECTION_IFRAME to true
  • Disconnect/reconnect connection
  • Secondary user connection
  • Connection on multisite

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration file.

@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Aug 18, 2020
@jetpackbot
Copy link
Collaborator

jetpackbot commented Aug 18, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16879

Scheduled Jetpack release: September 1, 2020.
Scheduled code freeze: August 25, 2020

Generated by 🚫 dangerJS against 7b5e461

@sergeymitr sergeymitr added this to the 8.9 milestone Aug 18, 2020
kbrown9
kbrown9 previously approved these changes Aug 19, 2020
Copy link
Member

@kbrown9 kbrown9 left a comment

Choose a reason for hiding this comment

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

The unit tests pass, so I'm approving! I left comments about a few small improvements that I think we could make to the tests. They're not blockers, though, so merge if you'd like.

@kbrown9 kbrown9 added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Aug 19, 2020
Moving the assertions to the end of the tests to ensure that environment cleanup gets performed even if a test fails.
@sergeymitr
Copy link
Contributor Author

sergeymitr commented Aug 19, 2020

Thanks @kbrown9, that's a really good catch. Failing one test did lead to failing other tests too, and you end up not knowing where to look ¯\_(ツ)_/¯
I commited the changes, please re-review.

@sergeymitr sergeymitr added [Status] Needs Review This PR is ready for review. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 19, 2020
Copy link
Member

@kbrown9 kbrown9 left a comment

Choose a reason for hiding this comment

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

LGTM!

@kbrown9 kbrown9 added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Aug 19, 2020
@sergeymitr sergeymitr merged commit b4f86e7 into master Aug 19, 2020
@sergeymitr sergeymitr deleted the update/remote-authorize-test-rebuilt branch August 19, 2020 19:07
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 19, 2020
davidlonjon added a commit that referenced this pull request Aug 20, 2020
* master: (23 commits)
  Premium Blocks: set blocks availability (#16898)
  Compat Package: Fix method declaration compatibility (#16900)
  Jetpack Dashboard: More meaningful error notices. (#16883)
  Connection REST API: Unit test for the `remote_authorize` request. (#16879)
  use blog token to request jetpack.updateBlog (#16698)
  Improve Story block media loading (#16663)
  Simplify error notices for broken connections (#16655)
  Use new heartbeat package (#16285)
  wrap-paid-block: remove component. deprecated. (#16895)
  Social Previews: improve preview description handling (#16889)
  Stats module use blog token (#16727)
  Form Block: add a new Consent Field, a new Newsletter setting, and a new newsletter variation (#16808)
  AAG: Backup card, fall back to VP content in case of /rewind API error. (#16867)
  Donations: Fix dependencies (#16892)
  Creative Mail: update option to lowercase (#16861)
  Premium Blocks: Implement the new design (#16611)
  Requests to Stats CSV use the blog token (#16716)
  Update spacing around sharing buttons to avoid no bottom margin below the customize link. (#16811)
  Jetpack SSO: Cleaning up the `requestNonce` API request. (#16830)
  Donations: Update plans when currency changes (#16844)
  ...
pereirinha pushed a commit that referenced this pull request Sep 10, 2020
…ze` request. (#16879)

The existing test only verifies the existence of the endpoint, and turned out to be unreliable.
The test has been rewritten to actually test the authorization process. Some existing tests were also refactored.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Connect Flow Connection banners, buttons, ... [Status] Needs Package Release This PR made changes to a package. Let's update that package now. Unit Tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants