vgteam / sequenceTubeMap

displays multiple genomic sequences in the form of a tube map
MIT License
180 stars 25 forks source link

Implement API Interface #381

Closed ducku closed 9 months ago

ducku commented 10 months ago

Creates APIInterface class, components that uses API calls now creates an instance of the class.

Future alternative solutions to API calls should be implemented in this class.

Closes #377

ducku commented 10 months ago

Not too sure why "renders with error when api call to server throws" is failing

I'm rethrowing caught errors in the class, and errors seem to be displayed correctly when I test manually.

Error

adamnovak commented 10 months ago

@ducku I think the test is failing because of the exciting way we try to mock fetchAndParse for all the React components:

import * as fetchAndParseModule from "./fetchAndParse";
...
jest.mock("./fetchAndParse");

beforeEach(() => {
  jest.resetAllMocks();
});
...
it("renders with error when api call to server throws", async () => {
  fetchAndParseModule.fetchAndParse = () => {
    throw new Error("Mock Server Error");
  };

It seems like we intend this to reach into the actual module and change the value of that function for everyone else who has also imported the same module, in e.g. the React components. This doesn't seem like it is supposed to work, and I don't think calling jest.mock() on the module is really going to solve that. It looks a lot like this deprecated approach that people say works on some transpiling setups and not others. I think it can't work if the import statements are truly implemented correctly.

I think we might be able to make replacing the function work if we do it as an argument to jest.mock(), because Jest will find that call and actually run it before doing the imports when it runs the test file, and it will make a fake mocked file to import instead of the real one. But then maybe we can only install one mock?

But once we get the APIInterface set up so that you can pass it down from the top, we won't need any mocks any more at all. We can just implement the interface with an implementation that throws, and pass that down to test the response of the application to exceptions on API calls.

adamnovak commented 10 months ago

But it looks like the real problem is that we're expecting the error to display synchronously, while really it is being detected and rendered in in the background and so we need to have code to wait for it to show up. I'm not sure why that worked before.

adamnovak commented 10 months ago

OK, I think I fixed the test. None of the mock-replacing stuff was actually required; I'm not entirely sure I want to keep it, since it's ugly. But maybe once the replaceable-API design is done we can drop the whole mocking exercise.

The real problem was hoping the error would render synchronously, which it doesn't seem to promise to do.

adamnovak commented 9 months ago

@ducku It looks like I missed that actually, even with this, we're still sending the raw apiUrl into HeaderForm: https://github.com/vgteam/sequenceTubeMap/blob/854896f5fe2bb02af62dab7b7dfaa15703ba62eb/src/App.js#L189

Which means it still talks to the API in ways other than by using the APIClient (for example, to do file uploads).

So I think #377 is really not done; we need to make all communication go through the APIClient.