Consistency checks for dynamic strings, before adding "Frère Jacques" song.

Ever since @pserwylo added the song-player in #28, I've wanted to add Frère Jacques ("are you sleeping, brother John"). I am 99% sure it has translations for all European languages, as since it definitely has Spanish, English and Chinese, we reach probably 75% of all living humans on the planet.

However, adding songs requires updating quite a few hardcoded strings in multiple places. Since I dislike "you just have to know where" as an answer, especially for open source projects hoping for new contributors, I've added unit tests that check these implicit consistency rules.

It's definitely not perfect, but I think it covers most cases:

all the reflection in here is a bit hairy.

Oh, I totally agree! This is the best I could come up with without additional dependencies, but it's definitely not up to my usual standards. To much "clever" and way too brittle.

Feel free to add commits on top, I think your greater experience here could teach me a thing or two! 💙

I am very hesitant to add dependencies for this, because everything I could think of (e.g. Class path scanning with ClassGraph) feels like way overkill. However, I may have to accept that Android Context is a complex beast, which requires big guns to deal with.

I'd be very happy with something more robust, but I am really running into my inexperience with the Android platform (PianOli is my first Android experience) . For example I've never heard of "robolectric" before. Their self-description on github sounds like it could be exactly what I was looking for (access context, but without emulator) , but I'm always a bit hesitant to believe the marketing blurbs. Are they really the "industry standard" like they claim? I lack the context to judge their plausibility.

One concern I have with robolectric: is it compatible with JUnit5? The examples I saw all work with JUnit4, and the plugin I added for 5 doesn't seem very "industry standard".

P.S. It may be good timing for context-mocking dependencies anyway. I'm running out of things that can be tested "pure java"-style. So far my refactoring managed to push context dependance "outwards" (Still good design, I think) , but that is reaching its own limits.

For sure, it is a wild west out there. I believe robolectric is "industry standard" for lack of a better word. If you google "Android tests" you go to the official docs for running local tests on the JVM: https://developer.android.com/training/testing/local-tests#dependencies. Note how this has a comment mentioning "robolectric environment".

And yes, it is still JUnit4. Perhaps that is why the aforementioned docs page has a dependency for JUnit4. However, a Google earlier showed that this seems likely to change, and so if we really want to stick with JUnit5, then we need junit-vintage-engine which seems like it is from the official JUnit5 folk. This lets you use the @RunsWith(RobolectricRunner.class) style of running tests while still using JUnit4. Seemed to me that it still downloads all of junit4 when doing a gradle build, which is a bit of a mess.

For what it is worth, I agree that pushing Context outward is a great idea. But as you also mentioned, you eventually run out of things that can be tested with pure Java.

So, some reading shows that robolectric has not been ported to junit5.

Robolectric is currently hard-coupled to the way junit4 does classloading. This mechanism could be ported to a junit5 extension, but nobody has prioritised this work in the last 6 years. I get the impression this is unlikely to change as long as junit5 is not officially supported by android itself.

I am not sure if this stackoverflow answer means that I could use the "vintage runner" for this specific test. However, I believethat would cost me the nice @ParametrisedTest syntax. (I've written parametric tests in junit4 for work, it's really verbose...) https://stackoverflow.com/questions/46971063/how-to-run-a-junit5-test-with-android-dependencies-and-robolectric#51566798

All in all, I'm not too fond of committing to a junit4-only framework just yet. Junit5 is soooo much nicer that I want to do some more research before giving it up.

@pserwylo, how firm is your opposition to the specific forms of reflection? Maybe we can find a mutually-acceptable subset for now, and postpone the truly hairy things to a separate MR.

To make my own hairyness-judgements explicit:

I am not at all concerned with getTranslationsByPrefix. Since I wrote this against the android-generated R-class, I think this is robust and future-proof, as the R-class is a pretty core part of the Android APIs.
- It is perhaps a bit weird to get the translation-id's via the java.reflection.Field name, but I feel that is a far smaller gun than a full-blown InstrumentationTest (Emulator/Device) or Robolectric (JVM) for the same, so a small bit of weirdness is acceptable to me.
checking melody-translations against Melodies.all has me unconcerned.
- The test-code is using the same "this melody exists" definition as the productive preferences code, I don´t see how to be cleaner than this.
- this can be clean because it delegates all the hard stuff to keeping the .all list updated.
  And indeed, testing that is a mess (see below)
Theme-fetching by scanning for static fields is definitely hacky, but I think that can be solved by cleaning up Theme itself to be an enum.
That would replace the reflective field-iteration with Theme.values(), which I think is the best possible form for our current Theme-needs. (current needs == hardcoded themes only; if we ever go dynamic or user defined, enums are not the right tool anymore, but until then: YAGNI)
- move Theme.fromPreferences to Preferences (merging it with Preferences.selectedTheme, which currently returns String), to mirror how we do Melody and SoundSet in there.
  Coincidentally, this would make Theme "android-free".
- convert each static Theme field to enum-values.

That leaves the two that I find actively ugly: the filesystem listers.
I´d really like a better solution to this, but I don´t see a "halfway" solution between what I have now and full-blown emulation/scanning for either of them..

.getSoundSets
(Path soundAssets = Paths.get("src/main/assets/sounds");)
- I don´t have a problem with the folder per-se, since it's the canonical folder used for soundset loading.
  - Although I could consider making the sounds/-path a re-usable constant in SampledSoundSet, instead of a magic string here; the relative path to the assets folder itself is masked by AssetManager in productive code.
- The big, philosophical question is: how much of the android system is in-scope of this test?
  - either we do this (relatively simple) filesystem scan (bypassing all of android)
  - or we say the android asset machinery (AssetManager) is "in scope" of the test. Which puts us firmly into "big gun" territory, because we've just added about a gazzilion lines of android-framework to our to-be-tested scope.
    - note to self: if we go this way, we can get the test-AssetManager via: ApplicationProvider.getApplicationContext().getAssets(); (needs dependency testImplementation 'androidx.test:core:1.5.0' )
MelodyTest.enumerateAllSongFiles (Path songSourceFolder = Paths.get("src", "main", "java", "com/nicobrailo/pianoli", "song");)
- since this is all our own code, we don't have to worry about Android-emulation, which is a relief.
- I strongly dislike the core assumption that all future songs will be in this folder.
- In fact, songs in the "wrong" folder is something I'd actually prefer to handle in this (or a companion) test.
  However, the only solution to that is a full scan of all folders and/or a full classpath-scan. (not just a classpath-scan, we´d have to actively load all classes to see if they define a Melody-field, ugh)

Time for bed now (reaaaaally, 3:09 in the night), but I wanted to bang out the Theme-as-Enum idea (#89).

(Bonus: coding with a sleeping baby on your lap grants the Wife some uninterrupted sleep)

nicolasbrailo / PianOli

Consistency checks for dynamic strings, before adding "Frère Jacques" song. #88