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

Added e2e tests for page.published webhook #15662

Closed
wants to merge 3 commits into from

Conversation

Dark-Knight11
Copy link
Contributor

@Dark-Knight11 Dark-Knight11 commented Oct 19, 2022

refs: #15537

This adds an e2e test and test snapshot for the page.published webhook so we can prevent regressions and bugs in the future

Got some code for us? Awesome 🎊!

Please include a description of your change & check your PR against this list, thanks!

  • There's a clear use-case for this code change, explained below
  • Commit message has a short title & references relevant issues
  • The build will pass (run yarn test:all and yarn lint)

We appreciate your contribution!

Also, if you'd be interested in writing code like this for us more regularly, we're hiring:
https://careers.ghost.org

@Dark-Knight11
Copy link
Contributor Author

I need some help with this test
My test for page.published event runs successfully when I run yarn test:single test/e2e-webhooks/page.tests.js
But it fails on yarn test:all
yarn lint also passes all tests
Not sure what's the issue here

@naz naz self-assigned this Oct 20, 2022
@naz
Copy link
Contributor

naz commented Oct 20, 2022

Hey @Dark-Knight11 the issue you are running into might have to do with the "authors" matcher behaving slightly differently when run solo or as a part of all suites. This same issue was discussed deeper in a recent PR. The solution might be adding anyLocalUrl matcher to the authors property matcher. Let me know if that works!

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Base: 52.75% // Head: 52.76% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (664bf5e) compared to base (0d3d85d).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15662   +/-   ##
=======================================
  Coverage   52.75%   52.76%           
=======================================
  Files        1468     1468           
  Lines       95976    95976           
  Branches    10740    10740           
=======================================
+ Hits        50636    50640    +4     
+ Misses      44092    44087    -5     
- Partials     1248     1249    +1     
Impacted Files Coverage Δ
ghost/admin/app/controllers/offer.js 42.10% <0.00%> (+0.52%) ⬆️
ghost/admin/app/components/gh-site-iframe.js 45.45% <0.00%> (+2.27%) ⬆️
ghost/admin/app/helpers/gh-price-amount.js 66.66% <0.00%> (+22.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Dark-Knight11
Copy link
Contributor Author

Hey @Dark-Knight11 the issue you are running into might have to do with the "authors" matcher behaving slightly differently when run solo or as a part of all suites. This same issue was discussed deeper in a recent PR. The solution might be adding anyLocalUrl matcher to the authors property matcher. Let me know if that works!

Nope that didn't work

@ErisDS
Copy link
Member

ErisDS commented Oct 21, 2022

Looks to me like you need to update your snapshots?

.body({
pages: [
{
title: 'testing page.published webhook',
Copy link
Contributor

Choose a reason for hiding this comment

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

The snapshot tests here are failing most likely because the post title is a bit long and the "anyLocalUrl" matcher did not handle it properly (here is a fix for it form another PR). to make things easier, might just give the page a single-word title OR reuse the fix from the linked above PR

@github-actions
Copy link
Contributor

Note from our bot: Some changes have been requested on this pull request. Updating your code is great, but won't notify us, so please leave a comment so that we (and our bot) can see when you've made the changes. Thank you 🙏

@Dark-Knight11
Copy link
Contributor Author

I think I'm facing some other issue over here

page.* events
       page.published event is triggered:
     "response.body" does not match snapshot.

Snapshot name: `page.* events page.published event is triggered 2: [body] 1`

- Expected properties  - 7
+ Received value       + 7

@@ -14,13 +14,13 @@
            ],
            "updated_at": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
            "url": StringMatching /http:\\/\\/127\.0\.0\.1:2369\\/\\w\+\\//,
          },
        ],
-       "comment_id": StringMatching /\[a-f0-9\]\{24\}/,
-       "created_at": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
-       "id": StringMatching /\[a-f0-9\]\{24\}/,
+       "comment_id": "635647ba4af07d143fc522eb",
+       "created_at": "2022-10-24T08:07:22.000Z",
+       "id": "635647ba4af07d143fc522eb",
        "primary_author": Object {
          "created_at": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
          "last_seen": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
          "roles": Array [
            Object {
@@ -30,11 +30,11 @@
            },
          ],
          "updated_at": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
          "url": StringMatching /http:\\/\\/127\.0\.0\.1:2369\\/\\w\+\\//,
        },
-       "published_at": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
+       "published_at": "2022-10-24T08:07:22.000Z",
        "tiers": Array [
          Object {
            "created_at": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
            "id": StringMatching /\[a-f0-9\]\{24\}/,
            "updated_at": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
@@ -43,13 +43,13 @@
            "created_at": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
            "id": StringMatching /\[a-f0-9\]\{24\}/,
            "updated_at": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
          },
        ],
-       "updated_at": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
-       "url": StringMatching /http:\\/\\/127\.0\.0\.1:2369\\/\\w\+\\//,
-       "uuid": StringMatching /\[a-f0-9\]\{8\}-\[a-f0-9\]\{4\}-\[a-f0-9\]\{4\}-\[a-f0-9\]\{4\}-\[a-f0-9\]\{12\}/,
+       "updated_at": "2022-10-24T08:07:22.000Z",
+       "url": "http://127.0.0.1:2369/page-published/",
+       "uuid": "e07748f9-c5a1-4cef-9235-2307f9884178",
      },
      "previous": Object {
        "tiers": Array [
          Object {
            "created_at": StringMatching /\\d\{4\}-\\d\{2\}-\\d\{2\}T\\d\{2\}:\\d\{2\}:\\d\{2\}\\\.000Z/,
  AssertionError [ERR_ASSERTION]: undefined undefined undefined
      at new AssertionError (node:internal/assert/assertion_error:467:5)
      at WebhookMockReceiver.matchBodySnapshot (/workspace/Ghost/node_modules/@tryghost/webhook-mock-receiver/lib/webhook-mock-receiver.js:53:23)
      at Context.<anonymous> (test/e2e-webhooks/pages.test.js:124:14)

"server": {
"host": "0.0.0.0"
}
}
Copy link
Member

@ErisDS ErisDS Oct 24, 2022

Choose a reason for hiding this comment

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

This should not be in the commit 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh sorry my bad
it wasn't there before though
I'll remove it

@ErisDS
Copy link
Member

ErisDS commented Oct 24, 2022

Just to be clear - the above comment from Naz is correct, the tests are failing because your snapshots need to be regenerated.

As you work through these kinds of changes, the snapshots need to be regenerated regularly, especially as there is more than one person working on this - it means we're always adding new snapshots and in this case, there's been a change to one of the matcher regexes.

Dark-Knight11 and others added 2 commits October 25, 2022 10:06
ref TryGhost#15537

- this adds an e2e test and test snapshot for the page.published webhook
so we can prevent regressions and bugs in the future
@Dark-Knight11
Copy link
Contributor Author

Just to be clear - the above comment from Naz is correct, the tests are failing because your snapshots need to be regenerated.

As you work through these kinds of changes, the snapshots need to be regenerated regularly, especially as there is more than one person working on this - it means we're always adding new snapshots and in this case, there's been a change to one of the matcher regexes.

Yeah but like I do test them on updated snapshots and the tests still fail for test:all and always pass for test:single

@Dark-Knight11
Copy link
Contributor Author

fixed the issue 👍

@Dark-Knight11 Dark-Knight11 requested a review from naz October 25, 2022 05:16
@Dark-Knight11
Copy link
Contributor Author

closing it as someone already made a PR for it

@Dark-Knight11 Dark-Knight11 deleted the page-published branch November 1, 2022 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants