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

tomasciccola · 2024-06-12T14:31:11Z

should close #482

Additionally this adds code to MapeoProject. Specifically:

Expose translation as a getter (only used in tests, but may be useful in the future)
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?

Additionally: * added `translation` getter in `mapeoProject` * `deleteAll(translation)` in import config

…onfigImporte2eTests

…ields or presets

EvanHahn

I'll give this a more thorough review on Monday, but found a few small things for now.

src/mapeo-project.js

test-e2e/config-import.js

* make `deleteTranslations` a closure, pass the logger * `deleteTranslations` avoid for loop, use Promise.all(map) * make failing test case instead of logging

src/mapeo-project.js

EvanHahn

Looks like tests are failing. Could you fix those?

tomasciccola · 2024-06-18T14:10:28Z

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:

Delete this test, we shouldn't expect a config being loaded twice in parallel
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

EvanHahn

I like the way you've enforced that imports only happen sequentially. Just a few more little things from me.

src/mapeo-project.js

Co-authored-by: Evan Hahn <[email protected]>

…core-next into feat/configImporte2eTests

EvanHahn

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

tomasciccola · 2024-06-19T17:23:19Z

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??

test-e2e/config-import.js

Co-authored-by: Evan Hahn <[email protected]>

…onfigImporte2eTests

test-e2e/config-import.js

EvanHahn

LGTM once we fix tests.

Tomás Ciccola added 3 commits June 12, 2024 11:26

add e2e tests for config-import.

2b158e0

Additionally: * added `translation` getter in `mapeoProject` * `deleteAll(translation)` in import config

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

ff954e4

…onfigImporte2eTests

rename test

318f716

tomasciccola requested a review from EvanHahn June 12, 2024 14:33

Tomás Ciccola added 4 commits June 12, 2024 12:02

deleteTranslations only delete translations that refer to deleted f…

de40cb9

…ields or presets

better signature for delete functions, simplify deleteTranslations

84b7dd0

simplify types

fcda6ae

better error text

868271c

EvanHahn requested changes Jun 13, 2024

View reviewed changes

src/mapeo-project.js Outdated Show resolved Hide resolved

src/mapeo-project.js Outdated Show resolved Hide resolved

src/mapeo-project.js Outdated Show resolved Hide resolved

test-e2e/config-import.js Outdated Show resolved Hide resolved

addressing review

002ca99

* make `deleteTranslations` a closure, pass the logger * `deleteTranslations` avoid for loop, use Promise.all(map) * make failing test case instead of logging

tomasciccola requested a review from EvanHahn June 13, 2024 17:36

EvanHahn reviewed Jun 17, 2024

View reviewed changes

src/mapeo-project.js Outdated Show resolved Hide resolved

EvanHahn requested changes Jun 17, 2024

View reviewed changes

revert deleteTranslations being a closure

4d40fc6

tomasciccola requested a review from EvanHahn June 18, 2024 14:22

add boolean to avoid loading a config in parallel

c6d8e39

EvanHahn requested changes Jun 19, 2024

View reviewed changes

src/mapeo-project.js Outdated Show resolved Hide resolved

src/mapeo-project.js Outdated Show resolved Hide resolved

src/mapeo-project.js Outdated Show resolved Hide resolved

src/mapeo-project.js Outdated Show resolved Hide resolved

tomasciccola and others added 4 commits June 19, 2024 13:07

refactor deleteTranslations

d3356f6

Co-authored-by: Evan Hahn <[email protected]>

wrap importConfig in a try/catch

6652190

Merge branch 'feat/configImporte2eTests' of github.com:digidem/mapeo-…

97707b5

…core-next into feat/configImporte2eTests

re-invert logic in deleteTranslations to please typescript

9bc8e72

EvanHahn approved these changes Jun 19, 2024

View reviewed changes

Tomás Ciccola added 2 commits June 19, 2024 14:20

fix logic error, change assertion on parallel test

8a1e208

expose translationApi.dataType

3f93439

tomasciccola commented Jun 19, 2024

View reviewed changes

test-e2e/config-import.js Outdated Show resolved Hide resolved

tomasciccola requested a review from EvanHahn June 19, 2024 17:23

fix type error

74cbf6e

EvanHahn requested changes Jun 24, 2024

View reviewed changes

test-e2e/config-import.js Outdated Show resolved Hide resolved

tomasciccola and others added 3 commits June 24, 2024 13:24

assert.throws -> assert.rejects

a01651c

Co-authored-by: Evan Hahn <[email protected]>

Merge branch 'main' of github.com:digidem/mapeo-core-next into feat/c…

cfdba9b

…onfigImporte2eTests

change test message

bcbf119

EvanHahn reviewed Jun 24, 2024

View reviewed changes

test-e2e/config-import.js Outdated Show resolved Hide resolved

EvanHahn approved these changes Jun 24, 2024

View reviewed changes

fix last test

6f58b33

tomasciccola merged commit 42bc021 into main Jun 25, 2024
7 checks passed

tomasciccola deleted the feat/configImporte2eTests branch June 25, 2024 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tomasciccola commented Jun 12, 2024 •

edited

Loading

EvanHahn left a comment

EvanHahn left a comment

tomasciccola commented Jun 18, 2024 •

edited

Loading

EvanHahn left a comment

EvanHahn left a comment

tomasciccola commented Jun 19, 2024

EvanHahn left a comment

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

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

Conversation

tomasciccola commented Jun 12, 2024 • edited Loading

EvanHahn left a comment

Choose a reason for hiding this comment

EvanHahn left a comment

Choose a reason for hiding this comment

tomasciccola commented Jun 18, 2024 • edited Loading

EvanHahn left a comment

Choose a reason for hiding this comment

EvanHahn left a comment

Choose a reason for hiding this comment

tomasciccola commented Jun 19, 2024

EvanHahn left a comment

Choose a reason for hiding this comment

tomasciccola commented Jun 12, 2024 •

edited

Loading

tomasciccola commented Jun 18, 2024 •

edited

Loading