search

MichaelSel / edoJS

A set of functions for manipulating musical pitches within a given EDO
GNU Affero General Public License v3.0
5 stars 0 forks source link

[Code] Automated tests? #8

Closed napulen closed 2 years ago

napulen commented 3 years ago

This issue is related to the following review: https://github.com/openjournals/joss-reviews/issues/3784

According to the JOSS guidelines, the software is expected to have automated tests.

I think, given that every function in the API seems to have an example with expected output, that this is not very far from implemented. However, it'd be great if an automated test could run the actual/expected outputs of each function.

MichaelSel commented 3 years ago

This is something that's been on my mind. It's quite tricky to test. I can create deterministic tests where I check for the expected output for a given input for every function. However, this is no guarantee that the code will work correctly with other inputs. Of course it would have been ideal to generate automated testing with random inputs, but there's no way to cross-check that the outputs are actually correct.

Another challenge is that some of the functions do not return deterministic outputs (random generators and such), and therefore, there's no way to verify their output.

I feel quite out of my comfort zone as I do not have vast experience in testing, and I would very much appreciate some guidance if you're willing to offer it.

Thanks!

napulen commented 2 years ago

I can create deterministic tests where I check for the expected output for a given input for every function.

Yes, that'd be unit testing. Highly recommend that you do that from your (already present in the documentation) input-expectedoutput examples.

However, this is no guarantee that the code will work correctly with other inputs.

Unfortunately, that'd always be the case. But still, even a few examples can get you a long way detecting changes in the code that have an unintended effect on your units.

Of course it would have been ideal to generate automated testing with random inputs, but there's no way to cross-check that the outputs are actually correct.

I think the few examples you already have are a good place to start. I wouldn't overcomplicate things. If anything, instead of one input-expectedoutput example, I'd add one or two more.

Another challenge is that some of the functions do not return deterministic outputs (random generators and such), and therefore, there's no way to verify their output.

For those cases, you can set the random seed before you run the test. Then your output is deterministic.

I feel quite out of my comfort zone as I do not have vast experience in testing, and I would very much appreciate some guidance if you're willing to offer it.

I was looking at a few projects of my peers. Seems people like to use jest. A quick Google query also showed that this seems to be the most popular library for unit testing in javascript. Seems relatively simple. I'd probably parse the examples in your documentation and generate the test .js files from them. Seems doable and not incredibly time consuming.

I hope this is helpful!

MichaelSel commented 2 years ago

I added unit tests to the vast majority of the functions (145 functions now pass a unit test).

I did leave 26 functions without a test. Those do not have a test because they are either outputting a random output (unfortunately JavaScript does not allow to set a random seed) image

OR, they have complicated outputs that didn't feel as urgent for this initial release (file outputs, image outputs, graphical outputs, huge data structures, etc.).

I hope you and Brian will find the existing unit testing that I added sufficient for now.

napulen commented 2 years ago

The tests look good. I want to run them locally before closing the issue. Will try to do that tonight.

napulen commented 2 years ago

Trying to run the tests locally, following the instructions of the README

npm install edo.js --dev
npm test

results in the following error:


> edo.js@1.2.16 test
> mocha

Error: Cannot find module './dist/edo.umd.js'
Require stack:
- /home/napulen/dev/edoJS/index.js
- /home/napulen/dev/edoJS/test/unit.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/home/napulen/dev/edoJS/index.js:3:15)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at unit_tests (/home/napulen/dev/edoJS/test/unit.js:2:17)
    at Object.<anonymous> (/home/napulen/dev/edoJS/test/unit.js:318:1)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:190:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:185:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:281:24)
    at async importModuleDynamicallyWrapper (node:internal/vm/module:437:15)
    at async formattedImport (/home/napulen/dev/edoJS/node_modules/mocha/lib/nodejs/esm-utils.js:7:14)
    at async Object.exports.requireOrImport (/home/napulen/dev/edoJS/node_modules/mocha/lib/nodejs/esm-utils.js:48:32)
    at async Object.exports.loadFilesAsync (/home/napulen/dev/edoJS/node_modules/mocha/lib/nodejs/esm-utils.js:88:20)
    at async singleRun (/home/napulen/dev/edoJS/node_modules/mocha/lib/cli/run-helpers.js:125:3)
    at async Object.exports.handler (/home/napulen/dev/edoJS/node_modules/mocha/lib/cli/run.js:374:5)

This is on

napulen commented 2 years ago

There is no dist/ folder generated by neither npm install nor npm install --dev.

After looking at the code, I noticed that the folder is created with the rollup.config.js script.

Using the documentation on rollup, I ran the config

npm install --global rollup
rollup --config rollup.config.js

This generated the dist/ folder. After that, the tests pass

$ npm test

> edo.js@1.2.16 test
> mocha

  EDO Class Unit Tests
    EDO.convert
      Deterministic tests in 12EDO
        ✔ cents_to_ratio
        ✔ cents_to_simple_ratio
        ✔ interval_to_cents
        ✔ interval_to_ratio
        ✔ intervals_to_pitches
        ✔ intervals_to_scale
        ✔ midi_to_intervals
        ✔ midi_to_name
        ✔ midi_to_freq
        ✔ pc_to_name
        ✔ pitches_to_PCs
        ✔ ratio_to_cents
        ✔ ratio_to_interval
        ✔ to_steps
    EDO.count
      Deterministic tests in 12EDO
        ✔ common_tones
        ✔ differences
        ✔ pitches
    EDO.get
      Deterministic tests in 12EDO
        ✔ angle
        ✔ coordinates
        ✔ contour
        ✔ combinations
        ✔ complementary_interval
        ✔ complementary_set
        ✔ harp_pedals_to_pitches
        ✔ interval_class
        ✔ n_choose_k
        ✔ notes_from_cents
        ✔ sine_pair_dissonance
        ✔ resize_melody
        ✔ generated_scale
        ✔ generators
        ✔ intersection
        ✔ interval_traversed
        ✔ interval_stack
        ✔ inversion
        ✔ levenshtein
        ✔ maximal_rahn_difference
        ✔ maximal_carey_coherence_failures
        ✔ minimal_voice_leading
        ✔ modes
        ✔ necklace
        ✔ new_pitches
        ✔ ngrams
        ✔ normal_order
        ✔ stacked
        ✔ without
        ✔ partitioned_subsets
        ✔ permutations
        ✔ pitch_distribution
        ✔ pitch_fields
        ✔ ratio_approximation
        ✔ retrograde
        ✔ rotated
        ✔ rotations
        ✔ shortest_path
        ✔ starting_at
        ✔ subset_indices
        ✔ subsets
        ✔ transposition
        ✔ union
        ✔ unique_elements
        ✔ well_formed_scale
        ✔ without_chromatic_notes
    EDO.is
      Deterministic tests in 12EDO
        ✔ element_of
        ✔ rotation
        ✔ same
        ✔ subset
        ✔ transposition

  Scale Class Unit Tests
    Scale.count
      Deterministic tests in 12EDO
        ✔ chord_quality
        ✔ consecutive_steps
        ✔ imperfections
        ✔ count
        ✔ M3s
        ✔ m3s
        ✔ major_minor_triads
        ✔ modes
        ✔ n_chords
        ✔ P5s
        ✔ pitches
        ✔ rahn_differences
        ✔ rahn_contradictions
        ✔ rahn_ambiguities
        ✔ rotational_symmetries
        ✔ tetrachords
        ✔ thirds
        ✔ trichords
        ✔ unique_elements
    Scale.get
      Deterministic tests in 12EDO
        ✔ area
        ✔ diagnostic_intervals
        ✔ set_difference
        ✔ per_note_set_difference
        ✔ coordinates
        ✔ common_tone_transpositions
        ✔ complement
        ✔ chord_quality_from_shape
        ✔ melody_from_intervals
        ✔ generic_intervals
        ✔ specific_intervals
        ✔ interval_vector
        ✔ inversion
        ✔ lerdahl_attraction
        ✔ lerdahl_attraction_vector
        ✔ levenshtein
        ✔ myhill_property
        ✔ modes
        ✔ n_chords
        ✔ neighborhood
        ✔ normal_order
        ✔ permutations
        ✔ pitches
        ✔ position_of_quality
        ✔ prime_form
        ✔ product
        ✔ rotations
        ✔ rothenberg_propriety
        ✔ roughness
        ✔ sameness_quotient
        ✔ coherence_quotient
        ✔ scale_degree_transpositions
        ✔ segments
        ✔ sequence_transposition
        ✔ stacks
        ✔ step_sizes
        ✔ steps_to_qualities
        ✔ supersets
        ✔ tetrachords
        ✔ scale_degree_roles
        ✔ transposition
        ✔ transpositions_with_pitches
        ✔ trichords
        ✔ without
    Scale.is
      Deterministic tests in 12EDO
        ✔ deep
        ✔ distributionally_even
        ✔ in_lower_edos
        ✔ invertible
        ✔ maximally_even
        ✔ myhill_property
        ✔ mode_of
        ✔ MOLT
        ✔ normal_order
        ✔ one_of
        ✔ prime_form
        ✔ subset
    Scale.to
      Deterministic tests in 12EDO
        ✔ cents
        ✔ steps
napulen commented 2 years ago

The README should include the instructions related to rollup, so that users can generate the distribution files to run the tests.

MichaelSel commented 2 years ago

You are correct, the README wasn't complete. I updated it now.

In fact you don't need to do the rollup, all you need to do is run '''npm run build''' to build all of the sources.

Then everything should run as expected.

napulen commented 2 years ago

Confirming the build script runs on my end. Thanks!