search

digidem / mapeo-core-next

The upcoming version of Mapeo Core
MIT License
7 stars 1 forks source link

feat: add e2e tests for config-import. #700

Closed tomasciccola closed 1 week ago

tomasciccola commented 3 weeks ago

should close #482

Additionally this adds code to MapeoProject. Specifically:

  1. Expose translation as a getter (only used in tests, but may be useful in the future)
  2. Delete translations that refer to deleted presets and fields

There's a test case where I try to load the default config twice in parallel that results in - what I think - unwanted expectations. Basically we get double the presets, fields and translations since when each config is loaded there's no docs to delete. We should think if this is a valid (possible) case and how we can fix the code to avoid this?

tomasciccola commented 2 weeks ago

Looks like tests are failing. Could you fix those?

So, the issue with the failing test is the following: That test loads the config twice in parallel (using Promise.all) and results in presets, fields and translations being loaded to the database twice. Those doctypes should be deleted before loading a new config, but this doesn't happen when loading in parallel. So to fix this tests I'm thinking:

  1. Delete this test, we shouldn't expect a config being loaded twice in parallel
  2. Find a way to force sequential excecution (So, if you call Promise.all([project.importConfig, project.importConfig]) internally it would force sequetial execution). I don't know if we can actually enforce this. A weird way I'm thinking that can solve this would be having a boolean in the class (like this.#loadingConfig). Not ideal, but it solves this case
tomasciccola commented 2 weeks ago

LGTM once we move to this.$translation.dataType (happy to discuss alternatives) and fix the failing test.

I've changed the failing tests so that it now expects an error being throwed. Despite that, the test still fails... any pointers??