Add tests to Enhanced Cover Art Uploads

[ROpdebee]

Couple of things to note:

• Using pollyjs to record and replay HTTP interactions with the providers instead of actually doing network requests
• Depends on Add tests #68

I suppose we could add a more integration test-like workflow to CI to run pollyjs in passthrough mode so we're actually performing the network requests rather than always replaying, so that we can find out whether there are any upstream changes that need to be accounted for. Such tests should definitely not be run on every commit in a PR, because, among other things, they may take quite a long time. Perhaps instead we could instead only run them on pushes to main (i.e. after a PR merge) before deployment, and/or on a cron schedule (perhaps once a week or so).

Drafting to check whether this works properly in CI.

[ROpdebee]

Some additional context w.r.t. the Tidal changes: The old implementation could not be tested properly as it always landed on the captcha page, likely due to the absence of browser headers. I experimented a bit to check which headers needed to be set through curl, managed to get a working page, retried a couple times to double-check, and I got IP banned on tidal.com after 3 of those requests. It's likely because I was requesting only the main page, and not properly doing what a browser would do. There doesn't seem to be a straightforward way to get an IP ban lifted, it seems to be either waiting or contacting support (who will probably say you'd have to wait as well).

In any case, using that strategy in the userscript is a very bad idea if Tidal IP bans so quickly, so now we're using the API with one of their own keys, which has been present in their JS for at least 3 years.

[ROpdebee]

I've been doing some thinking on how to best approach the core (non-provider) code refactoring to improve testability, so I'll just summarise so I don't forget:

The main challenge with testing the core code in its current state is that it's fairly heavily coupled to the UI. Since we're using JSX with nativejsx, that's near impossible to test. nativejsx does something completely different to the usual JSX implementations: In conventional JSX, JSX code is converted to a sort of object describing the DOM elements, which should later be fed into a render call, whereas nativejsx transforms the JSX code into DOM elements directly, eliminating the need for render calls. The problem with testing code containing JSX is that the JSX of course needs to be transpiled. There are babel plugins to do this, and we can use those before giving the code to jest, but those plugins use conventional JSX transformations, not the nativejsx ones. What we could do is write some helper functions similar to React.createElement to create DOM elements without the need to render them, but we'd still be heavily tied to the UI code. Instead, we should refactor the core code to be UI-agnostic, i.e., all UI stuff into separate modules.

There are two options here:

1. Keep the ImageImporter class in index.ts, which then imports ui.tsx (or whatever UI module structure we go with), and have ImageImporter do the same thing it's currently doing: Set up the UI in its constructor, using functions now imported from ui.tsx.
2. Move ImageImporter into a separate module and remove all UI-related stuff from its implementation, and make index.ts do all the wiring of UI and implementation code. So index.ts imports ui.tsx and ImageImporter, and does setupPage(..., importer.addImages). ImageImporter can then be completely decoupled from any UI stuff and tested fully in isolation.

The problem with option 1 is that in order to test ImageImporter, we'd have to mock out ui.tsx, which won't be pretty, so I'd go with option 2.

I then foresee the following changes to make this work:

1. Setting up the paste box will likely remain more or less the same as what is happening now, since it's already using a callback anyway.
2. Setting up the buttons should not be done in setupPage anymore. We'll instead factor out the "detection" of providers and do the wiring in index.ts.
3. Filling in the upload form should be removed from ImageImporter, since that's UI stuff. ImageImporter.addImages should instead return the results (and probably renamed), index.ts should give those results to a UI function that fills the form.
4. StatusBanner has got to go, we'll replace it with a more robust logging setup where we can attach custom logging sinks to display messages.

W.r.t. that logging setup:

1. Homegrown implementation.
2. Would be a lib module.
3. Simple interface (debug, log, info, warn, error).
4. Attach custom logging sinks which can decide how to display the data. Examples include logging to console, logging to a UI element like a StatusBanner or a more advanced UI like the one used in the work codes script.
5. Possibly a singleton Logger instance exported from the lib. Since it's rolled up into the userscript, each script still gets its owner logger.

The ImageImporter could then probably just do logger.info('Added …'); instead of using the status banner, and index.ts would create and attach the status banner logging sink so it gets displayed on the page. We could possibly also use some constants that get replaced by babel to set a debug mode on or off depending on the build environment. Later on, we could replace the simple status banner with something akin to the message display used in the work codes script, so that we can display more than one message at a time and separate errors/warnings from informational messages (as suggested in #66).

[ROpdebee]

Btw, the reason CI isn't running on these changes yet is because of the merge conflict. I'll resolve those conflicts once #68 is merged. Rest assured that all tests are passing (at least on my machine).

[kellnerd]

Since it's rather boring to see all tests pass, I am just making a few of them fail deliberately while I am reviewing locally 😁. I think I have already reviewed most parts of this PR too (as soon as you have pushed new commits) while I am still busy with #68, but it's hard to see the progress before the other one is merged (GitHub does not track the "reviewed" status of files across PRs which are containing the same commits). It won't take long now until I am done with the other one and you can rebase this PR.

[ROpdebee]

I've pushed a couple of misc things that aren't directly related to testing, but would be nice to have in a next release. We should fix #79 before releasing though...

[kellnerd]

LGTM, I did continuously review your code changes as you pushed them, otherwise this huge amount of tests would have given me a jestphobia for sure.
I did also run the tests locally, checking out the HAR files made me realize that I never hit the Windows maximum path length limitation before, which I finally had to disable.
There are only two minor suggestions, otherwise there is not much to add from my side.

Thank you also for experimenting with a generic approach to logging with multiple sinks, this is something many userscripts could benefit from. I think this is one of the things that could also be included in a potential shared (MusicBrainz) userscript tools package.

[ROpdebee]

this huge amount of tests would have given me a jestphobia for sure.

I gotta admit I was getting burnt out after a while too. This PR became much larger than what I initially expected.

Thank you also for experimenting with a generic approach to logging with multiple sinks, this is something many userscripts could benefit from. I think this is one of the things that could also be included in a potential shared (MusicBrainz) userscript tools package.

Definitely. I'm sure there's like 500 different logging libraries for JS, but I didn't want to fatten up the built code even more by embedding 20KB of logging libraries 😅 The logger is fairly simplistic but it does the trick and for its current purposes it offers just the right amount of flexibility. I've gotten used to loguru's dead simple logging setup without boilerplate, so I tried to somewhat replicate that (but without a default sink, and it's a bit easier to set a log level in our logger).