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

The deploy tests need jsDelivr mocks #351

Closed
mbostock opened this issue Dec 7, 2023 · 3 comments
Closed

The deploy tests need jsDelivr mocks #351

mbostock opened this issue Dec 7, 2023 · 3 comments
Labels
bug Something isn’t working

Comments

@mbostock
Copy link
Member

mbostock commented Dec 7, 2023

There’s a bug with the deploy tests that they require that the build tests have previously run and cached the responses from the jsDelivr API. So if you run the deploy tests by themselves, they fail. I need to figure out how to compose the jsDelivr API mocks with the Observable API mocks that the deploy tests need.

@mbostock mbostock added the bug Something isn’t working label Dec 7, 2023
@mythmon
Copy link
Member

mythmon commented Dec 7, 2023

For the AI assist project, I built a set of helper functions that implement a Playwright-style dependency-injection system for the tests. The key benefits that I saw for it were that the test helpers (like the jsDelivr and Observable API mocks) could have their own dependencies, and that they could automatically clean up after themselves (another problem we have in the current situation).

I think that using that this "test helper" system could help with composing the HTTP mocks because both mocks could delegate making undici interceptor to another test helper that could smartly only make one per test, and automatically deregister it after all mocks are done.

Is that something that you'd be amenable to? I can sketch out a PR for it.

Edit: Here is the code I did for the AI assist project: https://github.com/observablehq/observablehq/tree/main/notebook-prompts/test/fixtures. It isn't well demonstrated there because there aren't a lot of tests, but I think it gets the point across.

@cinxmo
Copy link
Contributor

cinxmo commented Jan 19, 2024

@mythmon is this resolved with #552?

@mythmon
Copy link
Member

mythmon commented Jan 19, 2024

I think so, yes. I'll admit I didn't actually explicitly test though.

@mythmon mythmon closed this as completed Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn’t working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants