Add tests

[AB1908]

I hope I did this correctly. I have another file for testing utils and I'll push it after cleaning up. There's definitely lots of scope for improvement here. I also didn't include the package json and other config stuff since I'm not sure about the correct way to add those. We can commit those at the end I guess.

[mProjectsCode]

This looks awesome. :)
It would probably be best if you include all files and configs in the pr.

[AB1908]

Still have much to do and learn. Will slowly add the the other commits.

[AB1908]

My sleep has messed up so I haven't had the time or energy to work on this but I'll get to it within the next week hopefully. Thanks for the patience :)

[AB1908]

How does one correctly mock away this.plugin? My test for OMDB is failing because the URL declaration is looking for the API key. this.plugin.settings is evaling to undefined and I'm not sure how to create a mock property/object.

obsidian-media-db-plugin/src/api/apis/OMDbAPI.ts

Line 30 in e478875

const searchUrl = `http://www.omdbapi.com/?s=${encodeURIComponent(title)}&apikey=${this.plugin.settings.OMDbKey}`;

My (TS) test code looks like:

beforeEach(() => {
    fetchMock.resetMocks();
    let mock = jest.fn();
    OMDbInstance = new OMDbAPI(new mock());
})

test("searchByTitle behavior when given garbage data", async () => {
    fetchMock.mockResponseOnce(JSON.stringify({
        data: "string"
    }))
    await expect(async () => await OMDbInstance.searchByTitle("sample")).rejects.toThrow();
    expect(fetchMock).toHaveBeenCalledTimes(1);
});

and I'm getting this.plugin.settings === undefined.

[mProjectsCode]

I will take a look at it when I have time. I am currently busy for the next few days.

[mProjectsCode]

@AB1908
Sadly my PC broke. I have send it in for warrenty repair, but that means that i can't help you currently, since i only have my laptop, where nothing i would need is installed.

[AB1908]

Absolutely fine! I'm supposed to be the one contributing haha and I'm asking for help, which is hilarious. I did post on Discord but didn't get a response so I'll continue experimenting. This is one of those times where I feel like I've hit a brick wall and I need help from someone with more jest knowledge. Hope you get a fixed laptop soon!

[mProjectsCode]

I was just looking through the docs and maybe this can help you.
https://jestjs.io/docs/manual-mocks

[AB1908]

I've been trying to figure out how that works. I guess I'll need some more time. I'm working on a fix to the SRS plugin as well. Maybe I'll try tackling some easier tests just so I can continue making progress.

[AB1908]

I added a very lazy mock since I couldn't mock modules correctly. The obvious improvement is to do something like:

let pluginMock: MediaDbPlugin = jest.createMockFromModule("../main.ts");

but this seems to fail with (not exact error message but similar)

    Cannot find module 'obsidian' from 'src/settings/Settings.ts'

    > 1 | import {App, PluginSettingTab, Setting} from 'obsidian';
        | ^
      2 |
      3 | import MediaDbPlugin from '../main';
      4 | import {FolderSuggest} from './suggesters/FolderSuggest';

      at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:491:11)
      at Object.<anonymous> (src/settings/Settings.ts:1:1)  

One thing I'm confused about is that since searchByTitle needs this.plugin.settings.OMDbKey, does this mean that we should mock MediaDbPluginSettings as well so that that doesn't fail? It seems like a lot of setup to write and I'm probably missing something here.

[AB1908]

Hi again, it's been a while and progress has stalled but I've run into a few questions regarding the nature of the test suite. At the moment, I'm still focused on writing tests for the various APIs but I can't say they're quite necessary. The methods are mostly doing a bit of mapping and are private as well so do you think they are worth testing? Should we test a public method instead where the underlying implementation is in use but we don't necessarily care about its input?

On a separate note, I feel that there's way too much duplication in the API wrappers and I'm considering refactoring it but I want to complete the test suite before embarking on that. I think the hand written mocks still work for now so if you think we should continue with this, I'll write the rest of them out. I've spent a decent amount of time trying to do mocks the correct way but I run into import resolution issues, instantiation issues or I just plainly don't understand what's happening at all.

[CedricFinance]

Hi again, it's been a while and progress has stalled but I've run into a few questions regarding the nature of the test suite. At the moment, I'm still focused on writing tests for the various APIs but I can't say they're quite necessary. The methods are mostly doing a bit of mapping and are private as well so do you think they are worth testing? Should we test a public method instead where the underlying implementation is in use but we don't necessarily care about its input?

If you're talking about the API in src/api/apis. I think it can be useful to test the two methods they implement ( searchByTitleandgetById`). I made a PR to fix a bug with the SteamApi. I prefer testing public methods as it makes less brittle tests.

On a separate note, I feel that there's way too much duplication in the API wrappers and I'm considering refactoring it but I want to complete the test suite before embarking on that. I think the hand written mocks still work for now so if you think we should continue with this, I'll write the rest of them out. I've spent a decent amount of time trying to do mocks the correct way but I run into import resolution issues, instantiation issues or I just plainly don't understand what's happening at all.

You're right, it will help having some tests before embarking on a refactoring. Those hand written mocks will also serve as a documentation about what we expect from the APIs. When I fixed the issue with Steam's API, I wasn't sure if there was a typo in the code or if it was the API that changed. If you still have issues with import resolution, instantiation and so one don't hesitate to ask me, I might be able to help.

Note: even if you don't write all tests for all APIs, it will be very valuable. It will make it easier for the author or any contributors to add tests as you'll have done the heavy lifting of setting every things up.

[AB1908]

Thank you for the feedback and offer of help! I'm currently stuck rewriting a separate plugin but I'll get back on this soon!

[AB1908]

I've finally got a generic-ish test suite up and running. Will slowly add in phases.

  OMDbAPI
    √ searchByTitle behavior when given garbage data (15 ms)
    √ searchByTitle when fetch returns 401 (19 ms)
  MALAPI
    × searchByTitle behavior when given garbage data (5 ms)
    √ searchByTitle when fetch returns 401 (3 ms)
  LocGovAPI
    √ searchByTitle behavior when given garbage data (7 ms)
    √ searchByTitle when fetch returns 401 (58 ms)
  MusicBrainzAPI
    × searchByTitle behavior when given garbage data (7 ms)
    × searchByTitle when fetch returns 401 (12 ms)
  SteamAPI
    × searchByTitle behavior when given garbage data (4 ms)
    × searchByTitle when fetch returns 401 (3 ms)
  WikipediaAPI
    × searchByTitle behavior when given garbage data (4 ms)
    √ searchByTitle when fetch returns 401 (4 ms)

[AB1908]

Hmm just learnt that we should be using requestUrl. I'll think about how to mock that correctly next week.

[AB1908]

I've made more progress and will add some commits over the next couple of days. Here are results:

  OMDbAPI
    ✓ searchByTitle behavior when given garbage data (16 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 401 (6 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 200 (1 ms)
  MALAPI
    ✕ searchByTitle behavior when given garbage data (3 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 401 (3 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 200 (1 ms)
  LocGovAPI
    ✓ searchByTitle behavior when given garbage data (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 401 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 200 (1 ms)
  MusicBrainzAPI
    ✕ searchByTitle behavior when given garbage data (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 401 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 200 (1 ms)
  SteamAPI
    ✕ searchByTitle behavior when given garbage data (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 401 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403
    ✓ searchByTitle behavior when requestUrl/fetch returns 200 (1 ms)
  WikipediaAPI
    ✕ searchByTitle behavior when given garbage data (2 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 401 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403
    ✓ searchByTitle behavior when requestUrl/fetch returns 200 (1 ms)

[AB1908]

Many many thanks to @Fevol for handing me a requestUrl mock that wraps over jest-fetch-mock. Many thanks to AquaCat from Discord and @metruzanca for guiding me through discussions on testing.

Based on their guidance, I was also wondering if we should add a set of integration tests where we use real data. My chief concern was around jikan as there appears to be potential for deprecation but I don't see any immediate need.

With API tests added, what are the next set of components we should focus on?

Final output:

 OMDbAPI
    ✓ searchByTitle behavior when API returns garbage data (17 ms)
    ✕ searchByTitle behavior when requestUrl/fetch returns 401 (22 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 200 (2 ms)
  MALAPI
    ✕ searchByTitle behavior when API returns garbage data (3 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 401 (2 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 200 (1 ms)
  LocGovAPI
    ✕ searchByTitle behavior when API returns garbage data (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 401 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403
    ✕ searchByTitle behavior when requestUrl/fetch returns 200 (1 ms)
  MusicBrainzAPI
    ✕ searchByTitle behavior when API returns garbage data (2 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 401 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403
    ✓ searchByTitle behavior when requestUrl/fetch returns 200 (1 ms)
  SteamAPI
    ✕ searchByTitle behavior when API returns garbage data (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 401 (2 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403
    ✓ searchByTitle behavior when requestUrl/fetch returns 200 (1 ms)
  WikipediaAPI
    ✕ searchByTitle behavior when API returns garbage data (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 401 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 403 (1 ms)
    ✓ searchByTitle behavior when requestUrl/fetch returns 200

[AB1908]

One area I see for improvement are some of the MAL and OMDb tests as they could return several types of media. I'll think about this in the coming weeks.

[mProjectsCode]

Thanks a lot. From quickly looking over it i am only confused by the ~7k changes in package-lock.json, but they are probably just jest stuff. I will take a deeper look and hopefully merge it on Wednesday after my uni exam.