search

cloudflare / wildebeest

Wildebeest is an ActivityPub and Mastodon-compatible server
Other
2.04k stars 399 forks source link

fix: `/api/v1/instance` #363

Open DataDrivenMD opened 1 year ago

DataDrivenMD commented 1 year ago

Taken together, this PR will improve compatibility with 3rd-party apps, including IceCubes and Ivory

Relevant issues:

cc @xtuc @dario-piotrowicz: could either of you please take a look at this PR?

DataDrivenMD commented 1 year ago

@dario-piotrowicz Every playwright test from seo.spec.ts is failing but it's not clear to me why that's happening since I didn't touch the front-end code in this PR. Separately, I'm getting 4-6 playwright failures that don't seem to have any discernible reason-- when I manually follow the tests, the results are as expected; so, the assertions are failing prematurely.

Is there another branch that I should be using as the base for this PR?

cc @xtuc

DataDrivenMD commented 1 year ago

So, I reverted all the changes in this PR, updated from cloudflare/wildebeest/main and then re-ran the playwright tests, and 6 tests still failed. Then, I manually checked one of the failing tests, and the page rendered correctly. So, it seems that there's something wrong with the playwright configuration (perhaps the timeout period is too short?).

cc @xtuc @dario-piotrowicz

playwright_failing manual_validation
DataDrivenMD commented 1 year ago

Update: I fixed the problem by adjusting Playwright timeouts...and that surfaced an uncaught exception in my code, which I fixed.

dario-piotrowicz commented 1 year ago

@DataDrivenMD I'm so sorry I didn't manage to reply earlier 🙇

Regarding SEO, it should work on main, is your branch up to date with main? and/or have you solved the issue?

Regarding increasing the timeouts I really am not a big fan of that because the current timeouts were working just fine so if you need to increase them it would suggest that our code is getting slower or something like that (also they were longer at some point but it was getting annoying that we'd have to wait a while to see if they would fail so I did shorten the timeouts)

and that surfaced an uncaught exception in my code, which I fixed. 👈 I am not really sure how longer timeouts would surfaced the issue, can you tell me? 🙂 (and regarding this, before increasing the timeouts, have you tried viewing the remote playwright reports? didn't that help?)

DataDrivenMD commented 1 year ago

@DataDrivenMD I'm so sorry I didn't manage to reply earlier 🙇

No worries! Thanks for closing the loop.

and that surfaced an uncaught exception in my code, which I fixed. 👈 I am not really sure how longer timeouts would surfaced the issue, can you tell me? 🙂 (and regarding this, before increasing the timeouts, have you tried viewing the remote playwright reports? didn't that help?)

Prior to extending the timeouts I would see different e2e tests fail with every run (i.e. every time I called yarn playwright test). Without changing a single line of code, I ran the tests several times: sometimes 50+ tests would fail, sometimes 30-40, then 26, then 56. It was all over the place, and the tests in seo.spec.ts wouldn't always fail as a group.

Once I extended the timeout period, the playwright tests defined in other files (i.e. not seo.spec.ts were able to run completion (and PASS). Then, and only then, was I able to get consistent results between runs: all of the tests in seo.spec.ts began failing as group in a repeatable manner. That's what I mean by "surfaced an uncaught exception in my code".

IOW: the e2e tests were working as designed but the short timeout period triggered some of them to fail prematurely in a flaky manner. This, in turn, made it impossible to figure out whether the tests were failing due a bug in my code vs. failing due to resource constraints vs. some other reason. Once the timeout period was extended, it was possible to see that a) there was an actual bug in the code and b) trace the exception. The timeout issue is fixed in PR #375, which has now been merged to main

PS - Sorry for the confusion, and I hope my answer clarifies the issue

DataDrivenMD commented 1 year ago

@xtuc Just want to flag that this PR is ready for re-review. I left two comments unresolved but added explanations for the proposed changes.

dario-piotrowicz commented 1 year ago

@DataDrivenMD I'm so sorry I didn't manage to reply earlier 🙇

No worries! Thanks for closing the loop.

and that surfaced an uncaught exception in my code, which I fixed. 👈 I am not really sure how longer timeouts would surfaced the issue, can you tell me? 🙂 (and regarding this, before increasing the timeouts, have you tried viewing the remote playwright reports? didn't that help?)

Prior to extending the timeouts I would see different e2e tests fail with every run (i.e. every time I called yarn playwright test). Without changing a single line of code, I ran the tests several times: sometimes 50+ tests would fail, sometimes 30-40, then 26, then 56. It was all over the place, and the tests in seo.spec.ts wouldn't always fail as a group.

Once I extended the timeout period, the playwright tests defined in other files (i.e. not seo.spec.ts were able to run completion (and PASS). Then, and only then, was I able to get consistent results between runs: all of the tests in seo.spec.ts began failing as group in a repeatable manner. That's what I mean by "surfaced an uncaught exception in my code".

IOW: the e2e tests were working as designed but the short timeout period triggered some of them to fail prematurely in a flaky manner. This, in turn, made it impossible to figure out whether the tests were failing due a bug in my code vs. failing due to resource constraints vs. some other reason. Once the timeout period was extended, it was possible to see that a) there was an actual bug in the code and b) trace the exception. The timeout issue is fixed in PR #375, which has now been merged to main

PS - Sorry for the confusion, and I hope my answer clarifies the issue

Ah I see, ok I think I get it, you're talking about local runs right? maybe it just worked smoothly on my mac but on a different machine it could be not as reliable 🤔

Anyways thanks a bunch for the explanation 🙂

DataDrivenMD commented 1 year ago

Turns out there were 2 issues causing e2e tests to fail (now fixed):

  1. I needed to pass the DOMAIN=cloudflare.com binding to the e2e test suite in order to pull the instances stats. I added that binding to the yarn ci-dev-test-ui script in the package.json file.

  2. I needed to update the type definition used by the frontend to match the output from the Mastodon-compatible API

Separately, the TypeError that I shared above is still there but all tests are passing, so I don't know what to make of it. It seems to stem from the query builder code.

cc @dario-piotrowicz @xtuc

DataDrivenMD commented 1 year ago

Just upstreamed all the changes in this branch. Should be ready again for you @xtuc