-
Notifications
You must be signed in to change notification settings - Fork 64
Use a non-versioned API URL to mock analytics requests, too #1045
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to run talkback with the normal pnpm test:e2e
script? Or is it only possible with the CI version?
Looking good so far, just testing locally but will probably be able to approve soon.
test/proxy.js
Outdated
const recordMode = updatingTapes | ||
? talkback.Options.RecordMode.NEW | ||
: talkback.Options.RecordMode.DISABLED | ||
: talkback.Options.RecordMode.NEW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this make recordMode
always NEW
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also left-over from last-minute debugging :(
test/proxy.js
Outdated
port: 3000, | ||
port: 3322, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅 This seems like a better choice considering how many other things live on port 3000 by default anyway (I know a few that are missing from this list too, like Flask I think?)
https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers
In fact, I kind of wonder if we shouldn't switch all of our stuff to use ports in the completely unregistered "temporary services" range of 49152–65535.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
49152 it is, then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. I noticed that in addition to #985 talkback also didn't stop running when I ran pnpm ci:e2e-test
locally even though the tests passed and didn't crash or anything 😕 Did you notice that happening as well?
Oh I just noticed another tape was created when I ran |
I just spent 15 minutes chasing these new tapes. There is often 1 image or audio request that's new. I think it's one of the related images or audio that sometimes change. I guess we should open an issue to mock related image / audio requests... |
I think the fix I have in the proxy to replace API results that reference the upstream server with proxy references will address this, so I wouldn't sweat it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well, LGTM 👍
Fixes
Fixes #915 by @obulat
Description
This PR creates two ApiService objects: one is versioned, for most endpoints, and an unversioned one for the analytics endpoints.
Some data services were also refactored to reuse code from other services (
getMediaSlug
, for example), and thebug-report
was removed as it is unused.The proxy port was also changed from
3000
to3322
because when I was testing, the API was running in Docker, and3000
port was busy.It also renames the
update-tapes
torecreate-tapes
, and adds anupdate-tapes
script that does not completely delete all the tapes, but simply adds any tapes that are missing. This way, we can update a single tape when any e2e test changes, and don't have too many changes in a PR.Testing Instructions
Enable internal analytics:
openverse-frontend/src/utils/env.ts
Line 9 in 690f523
Run
pnpm run update-tapes
. This will delete all the tapes files, and, more importantly, it will save any response that isn't found intest/tapes
directory. You should see a lot of tapes for analytics responses. They are all 404, but they are there.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin